-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add and Mul are associative. Let's make them canonical #413
Conversation
src/EVM/Expr.hs
Outdated
@@ -880,6 +880,9 @@ simplify e = if (mapExpr go e == e) | |||
| b == c = a | |||
| otherwise = sub (add a b) c | |||
|
|||
-- Add is associative | |||
go (Add (Add a b) c) = add a (add b c) |
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 am wondering if there is a way to do this only if it is beneficial.
For instance if the initial expression is (Lit 17 + Lit 42) + x
then rearranging the expression will miss an opportunity for further simplification.
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.
Oh wow, good point! I think we should be canonical, so it should eventually always be done. But you are right that here we are very much missing an opportunity. What about this (for both Add
and Mul
):
-- Add is associative
go (Add (Add a b) c) = add a (add b c)
go (Add (Lit a) (Add (Lit b) x) = add (Lit (a+b) x
Notice that the second Add
will always have Lit
as 1st argument thanks to canonicalization done by add
(in case any of its arguments are a Lit
). The above ensures that if eventually somehow, the second Add
's any argument becomes a Lit
and the 1st Add
's 1st argument becomes a Lit
then we simplify.
This should work for Mul
as well. The only special case there is Mul
by Lit 0
and Lit 1
, but that work with and 2nd argument, so the 2nd argument can be another Mul
.
What do you think?
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.
Yes! I think adding this rule would be great!
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.
looks good!
Also fixing up the test that was broken
153a4ce
to
85301db
Compare
Thanks to @zoep for this!
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.
Looks great!
Description
As per @zoep 's idea,
Mul
is associative. So we should makeMul
+Mul
canonical. This fixes #408 . That issue also mentionsAdd
. Doing that too.I added an automated test for the
Mul
associativity, as that's not obvious I think. Anyway, now it's proven correct.Checklist