-
Notifications
You must be signed in to change notification settings - Fork 34
Support unary predicates in cat #908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I haven't checked the code in detail, but something I am not a big fan of is the "abuse" of the term relation even for unary predicates. I know that in some domains (e.g., mathematics) relations can be also unary, but I think the terms "relation" and "set" are much more common in the domain of WMMs. One possibility would be to use set/relation/predicate for the unary/binary/general cases, but this will probably require changes all over the place. |
The problem is that you would need two classes, one to represent sets, and one to represent relations. |
I do not see why that would contradict my proposal. That class could be called |
|
If you mean it like that, then sure. |
dartagnan/src/main/java/com/dat3m/dartagnan/encoding/EncodeSets.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/encoding/WmmEncoder.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/encoding/WmmEncoder.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/cat/VisitorCat.java
Outdated
Show resolved
Hide resolved
|
|
||
| private String createUniqueName(String name) { | ||
| if (namespace.containsKey(name) && !nameOccurrenceCounter.containsKey(name)) { | ||
| if ((namespace.containsKey(name) || wmm.getRelation(name) != null) && !nameOccurrenceCounter.containsKey(name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I encountered Already bound name W, and others like ext and figured that I probably should check the global namespace as well when computing unique names. A better way would be to add relations to the local namespace on every occasion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only case where relations are "secretely" generated (i.e. outside of the parser) is when invoking wmm.getPredefinedRelation (or whatever it is called). This one is a little annoying. I think for binary relations, we just opted to never generate named subrelations (except for the hidden one's like idd) so that there was never a name conflict.
I guess if you do the same with tag sets generated there, it would should work correctly. At least I don't see why the code that worked for binary relations shouldn't just work out-of-the-box for unary relations anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another Already bound name F occurred, because in opencl.cat, dynamic_tag(X) is defined before F is declared, which is used in that function. dynamic_tag is called twice, so the namespace lookup misses twice, leading to a redefinition of F.
Maybe createUniqueName should always check wmm, rather than namespace, to not be sensitive to local scopes and to guarantee uniqueness in wmm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot really follow. You removed the update of the namespace in visitExprBasic and that is why you create relations without registering their names in the namespace. If you do this correctly, and assuming that wmm.getOrCreatePredefinedRelation(k) does not create named subrelations for tag set, I think it should work just fine.
What breaks if you use the original 1-line code but replace the Filter.byTag(k) with addDefinition(new TagSet(wmm.newSet(k), k))?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What breaks if you use the original 1-line code but replace the
Filter.byTag(k)withaddDefinition(new TagSet(wmm.newSet(k), k))?
There are two loosely-dependent namespace systems at work here: Wmm.getRelation(String) and VisitorCat.namespace. During parsing, I get collisions in Wmm.addAlias(String,Relation), because the previous call to VisitorCat.createUniqueName(String) only checks the other namespace system.
Previously we used Wmm.addFilter(Filter) to bind a name to a set. That is a third namespace system and it does not check for name collisions. Now that sets and relations have merged, these collisions are detected. This also happens with the oneliner.
You removed the update of the namespace in visitExprBasic and that is why you create relations without registering their names in the namespace.
I removed the update in visitExprBasic, because I added an additional lookup with wmm.getRelation(name). With this, binding the name in the local namespace became obsolete. let instructions still modify that namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to understand how collisions even happen. I understand that there are two namespaces (the parser and the wmm), but from my understanding every name in the Wmm should also appear in the parser's namespace if visitExprBasic and visitLetExpr register all names they encounter, no? The only exception I can think of is if wmm.getOrCreatePredefinedRelation(k) creates named subrelations which then appear in the wmm's namespace but not in the parser's.
Either way, my main concern was that something might go wrong if the parser's namespace is not aligned with the wmm's namespace, especially when functions that capture only the parser's namespace are involved. But after thinking about it for a little bit, I don't think anything breaks there.
...n/src/main/java/com/dat3m/dartagnan/solver/caat4wmm/basePredicates/DynamicDefaultWMMSet.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/verification/solving/RefinementSolver.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/verification/solving/RefinementSolver.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/wmm/definition/TagSet.java
Outdated
Show resolved
Hide resolved
A quite annoying problem is that the |
With currently 569 usages of
Then I throw |
I prefer the first. Such "double naming" is never an issue IMO. EDIT: Also, I agree about delaying the renaming to another PR. |
78ea000 to
81e10dc
Compare
hernanponcedeleon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me forever to come back to this PR.
I left a few more comments
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/cat/VisitorCat.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/cat/VisitorCat.java
Outdated
Show resolved
Hide resolved
...n/src/main/java/com/dat3m/dartagnan/solver/caat4wmm/basePredicates/DynamicDefaultWMMSet.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/wmm/definition/SetIdentity.java
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/encoding/WmmEncoder.java
Outdated
Show resolved
Hide resolved
hernanponcedeleon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Unless @ThomasHaas has further comments, we can merge
Add predefined relations M and _ Restore composition-based relations in RefinementSolver.addBiases()
Rename 'new()' to 'newRelation()'.
e2c3beb to
d84adca
Compare
dartagnan/src/main/java/com/dat3m/dartagnan/solver/caat4wmm/ExecutionGraph.java
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/wmm/definition/Range.java
Outdated
Show resolved
Hide resolved
ThomasHaas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR affects the intermediate representation for memory models, such that
Relationcan model sets as well. This relates to issues #535 and #371.Features:
domain(r)andrange(r)without brackets.Newconstructs a free set.empty raxiom allows a set as operand.API changes:
Relation.isUnaryRelation()andRelation.isBinaryRelation().Constraint.Wmm.newRelation(boolean)andWmm.newRelation(String,boolean).DomainIdentityandRangeIdentitywithDomainandRange.SetIdentityas non-static.TagSetas its bottom-level replacement for staticSetIdentity.RelationAnalysisalso learnsKnowledgefor unary relations, stored as identity graphs.CAAT changes:
ProjectionIdentityGraphwithProjectionSet.IntersectionSetandUnionSet.SetPredicateonly depends onSetPredicate.AbstractWMMSet,DynamicDefaultWMMSetandMaterializedWMMSet.SetPredicatefor unary relations inExecutionGraph.Additional:
caatwhereElement.derivLengthwas not updated.