Skip to content
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

Tiny simplification of constFoldProp #530

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

blishko
Copy link
Collaborator

@blishko blishko commented Aug 19, 2024

Description

I am trying to learn how different pieces of hevm work and concurrently learning Haskell.
Here is my attempt to simplify a piece of code using monadic operations.

  • tested locally

src/EVM/Expr.hs Outdated
@@ -1575,8 +1569,7 @@ constFoldProp ps = oneRun ps (ConstState mempty True)
v2 = oneRun [b] s
unless v1 $ go b
unless v2 $ go a
s2 <- get
put $ s{canBeSat=(s2.canBeSat && (v1 || v2))}
put $ s{canBeSat=(s.canBeSat && (v1 || v2))}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do not need to read the state again, because we have the information where any of the two disjunct is inconsistent in the results v1 and v2. We can use the old value of s.canBeSat, no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not, actually! The oneRun can change it! If you do git blame it kinda tries to show it... maybe not too well. But yeah, so it's actually not correct. Kind of a minefield. Maybe you could put a comment here, since I have also made this mistake and I have a feeling someone else may also bump into this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on our offline discussion.
After some exchange of ideas, I believe that updating the state here is unnecessary.
The reasoning is that the correct update already happens in go statements (if they are executed).
It goes like this:

  • If both v1 and v2 are true, then both disjuncts are satisfiable, and neither go a or go b will be executed. So we just continue with the same state. No need for any update.

  • Suppose that one of the variables is false, say v1, but the other is true.
    That means that disjunct a is unsatisfiable, b is satisfiable. The code will execute go b. That should just keep the variable canBeSat of the current state the same, because we already know this branch is satisfiable. And, the map with values will be updated (which is something that is actually not happening in main). So we do not have to touch the state, it is already OK.

  • If both are unsatisfiable, both go b and go a will execute, and the latter will make the state unsat plus empty map. So again, no update is necessary afterwards.

@blishko blishko requested review from msooseth and d-xo August 19, 2024 13:38
@blishko blishko force-pushed the tiny-preprocessing-simplification branch from e0e0046 to fa56375 Compare August 19, 2024 13:40
@blishko blishko force-pushed the tiny-preprocessing-simplification branch from fa56375 to 13b73a4 Compare August 20, 2024 13:08
@blishko
Copy link
Collaborator Author

blishko commented Aug 20, 2024

@msooseth, I am gonna add more tests, and rename the method to isUnsat, because that is what the method actually checks, do you agree? It does not actually do constant folding.

@msooseth msooseth merged commit bcdc688 into main Aug 20, 2024
9 checks passed
@blishko blishko deleted the tiny-preprocessing-simplification branch August 20, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants