vyperlang / vyper

Pythonic Smart Contract Language for the EVM
https://vyperlang.org
Other
4.91k stars 804 forks source link

fix[venom]: add `make_ssa` pass after algebraic optimizations #4292

Closed harkal closed 1 month ago

harkal commented 1 month ago

What I did

Resolve #4288 caused by unnormalised code after store elimination manipulating phi instructions. Added a test case for it.

How I did it

Perform a MakeSSA pass after algebraic optimizations

How to verify it

Commit message

This commit adds a `MakeSSA` pass after algebraic optimisations. Makes
`StoreElimination` pass skip `phi` instructions and adds a test case
that would fail without this step. The `MakeSSA` pass here results in
smaller code so we are keeping it in for now.

Resolves GH issue #4288

Description for the changelog

Cute Animal Picture

![Put a link to a cute animal picture inside the parenthesis-->]()

charles-cooper commented 1 month ago

How do we get unnormalized code after algebraic optimizations?

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 50.86%. Comparing base (039d369) to head (674b203). Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
vyper/venom/passes/sccp/sccp.py 0.00% 3 Missing :warning:
vyper/venom/passes/store_elimination.py 25.00% 3 Missing :warning:
vyper/venom/analysis/cfg.py 0.00% 2 Missing :warning:
vyper/venom/__init__.py 0.00% 1 Missing :warning:

:exclamation: There is a different number of reports uploaded between BASE (039d369) and HEAD (674b203). Click for more details.

HEAD has 170 uploads less than BASE | Flag | BASE (039d369) | HEAD (674b203) | |------|------|------| ||187|17|
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4292 +/- ## =========================================== - Coverage 89.39% 50.86% -38.54% =========================================== Files 112 112 Lines 15923 15927 +4 Branches 2693 2694 +1 =========================================== - Hits 14235 8101 -6134 - Misses 1192 7179 +5987 - Partials 496 647 +151 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

charles-cooper commented 1 month ago

since MakeSSA is expensive, i think this solution works, but is not ideal. i'm ok with merging it as a kludge, but i think we want to leave a note to circle back and figure out why we are being taken out of ssa.