-
Notifications
You must be signed in to change notification settings - Fork 10
First stab at handling permutative rewrite rules #173
Conversation
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.
To avoid the performance hit, we should not evaluate rulePermutes
repeatedly during simplification; instead, let's evaluate it once and cache the permutes
bit alongside each rule in the simpset. We can probably just add a new Bool
field to the RewriteRule
type.
Comment from @msaaltink:
|
If |
I think first-order matching will be fine. I'm not really sure what a reasonable higher-order permutative rewrite rule would look like anyway. |
This now checks for permutativity when a RewriteRule is created. This is not as much as an improvement as one might have hoped, but appears to be slightly more efficient than the original. |
2576777
to
010e34e
Compare
In case it is helpful, here's a file of things I use to test the rewriter mods:
|
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.
This looks reasonable. One remaining uncertainty is about the design of the term ordering. If this term ordering works well enough for your current use case, then you're welcome to go ahead and merge this PR. But in any case, in the longer term, we should probably make a well-informed deliberate choice about the right term ordering to use. (Some example term orderings used in the Isabelle simplifier can be seen here: https://github.com/isabelle-prover/mirror-isabelle/blob/master/src/Pure/term_ord.ML)
If you don't want to address the term ordering right away, that's fine, but we should open a ticket so that we don't forget to revisit the issue.
I am in complete agreement about the term ordering. I had also looked at the term orderings used by ACL2 (a strange and very detailed ordering) and by the old EVES system (which used LRPO). The Isabelle term orderings are also worth looking at, thanks. A problem I had was in maintaining compatibility with the existing rewrite system, so that non-permuting rules are always applied. This can interact with well-founded term orderings in a bad way. So the current rule is a compromise, that works in the cases Giuliano and I want right now. But I agree, a ticket is appropriate, and some more careful design needed. As well, after this is merged the SAW documentation ought to be revised to describe this as, I suppose, an "experimental and subject to change" handling of looping rules. |
Giuliano and I have been working on some proofs that would be much easier if the rewriter could handle permutative rules. This small modification to the rewriter allows for associative and commutative functions to be handled, whether curried or not. It is a bit inefficient, adding about 3% to the proof time for a rewrite-intensive proof suite that does not use permuting at all, and could be optimized if that's a problem.
A rule is 'permutative' if the left and right sides are instances of one another. An ordering relation is then used to decide whether to apply the rule or not. I'm not sure what a general ordering rule would be for higher-order logic, and the ordering I implemented here works OK for association-type laws but is not as general as I would like. The ordering looks at the "fringe" of arguments in possibly-nested calls to a function. Possibly LRPO would be a better choice, but I would need to understand the saw-core term structure better to implement that.
Other than the 3% cost of this feature I think there is no downside; the rules it applies to would have looped in the existing rewriter and so whatever I am doing in no worse.
The code happens to recognize when a single rule could loop and could easily be adapted to block the application of such a rule. This would still not prevent loops involving more than one rule, but is a start.
I have a small test suite for this, but in saw-script rather than in Haskell, so I do not know how I could add it to the tests here.