Skip to content

fix[venom]: fix MakeSSA with existing phis#4423

Merged
charles-cooper merged 19 commits intovyperlang:masterfrom
harkal:fix/makessa_with_existing_phis
Dec 24, 2024
Merged

fix[venom]: fix MakeSSA with existing phis#4423
charles-cooper merged 19 commits intovyperlang:masterfrom
harkal:fix/makessa_with_existing_phis

Conversation

@harkal
Copy link
Copy Markdown
Collaborator

@harkal harkal commented Dec 24, 2024

What I did

Fixed the memssa pass to support already existing phi instructions.

How I did it

How to verify it

Commit message

This commit improves the `MakeSSA` pass to handle incoming `phi`
instructions. Following the implementation of the venom parser it
is possible to parse code with existing `phi` instructions in the
code. The old code was expecting the `phi` instructions to have
been self-placed and in "degenerate form" of output == all branch
arguments. The new code does not make this assumption.

This commit additionally:
- refactors the existing `make_ssa` tests to use new test machinery
- adds a phi parsing test (does not test the new code in this commit
  since it does not run passes, but does make sure we can at least parse
  phis)
- expands the venom round-trip tests to check that we can both
    a) run venom passes on parsed venom, and
    b) bytecode generation from round-tripping venom produced by the
       vyper frontend is equivalent to bytecode generation from the
       regular pipeline (directly from vyper source code)

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@harkal harkal marked this pull request as ready for review December 24, 2024 12:40
Comment on lines +238 to +241
# def _loop() -> uint256:
# res: uint256 = 9
# for i: uint256 in range(res, bound=10):
# res = res + i

Check notice

Code scanning / CodeQL

Commented-out code

This comment appears to contain commented-out code.
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 24, 2024

Codecov Report

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

Project coverage is 51.79%. Comparing base (9c98b3e) to head (52591ed).

Files with missing lines Patch % Lines
vyper/venom/basicblock.py 14.28% 6 Missing ⚠️
vyper/venom/passes/make_ssa.py 0.00% 3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (9c98b3e) and HEAD (52591ed). Click for more details.

HEAD has 113 uploads less than BASE
Flag BASE (9c98b3e) HEAD (52591ed)
140 27
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4423       +/-   ##
===========================================
- Coverage   91.76%   51.79%   -39.97%     
===========================================
  Files         119      119               
  Lines       16615    16615               
  Branches     2796     2796               
===========================================
- Hits        15247     8606     -6641     
- Misses        936     7364     +6428     
- Partials      432      645      +213     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@charles-cooper
Copy link
Copy Markdown
Member

i added more tests (1728bee runs venom passes in the round-trip tests) and there still seems to be some bugs, e.g. https://github.com/vyperlang/vyper/actions/runs/12483208026/job/34838622968?pr=4423 throws things like
Screenshot from 2024-12-24 10-43-14

at first glance, it looks like the fix fails if there are multiple phis in multiple basic blocks referencing the same variable name, but not sure

@charles-cooper charles-cooper changed the title fix[venom]: makessa with existing phis fix[venom]: fix MakeSSA with existing phis Dec 24, 2024
@charles-cooper charles-cooper changed the title fix[venom]: fix MakeSSA with existing phis fix[venom]: fix MakeSSA with existing phis Dec 24, 2024
@charles-cooper charles-cooper enabled auto-merge (squash) December 24, 2024 21:18
run without --experimental-codegen
@charles-cooper charles-cooper enabled auto-merge (squash) December 24, 2024 21:30
@charles-cooper charles-cooper merged commit c9d8f5b into vyperlang:master Dec 24, 2024
@charles-cooper charles-cooper deleted the fix/makessa_with_existing_phis branch December 24, 2024 23:50
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.

3 participants