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

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

Merged
merged 19 commits into from
Dec 24, 2024

Conversation

harkal
Copy link
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 Note test

This comment appears to contain commented-out code.
tests/functional/venom/test_venom_repr.py Fixed Show fixed Hide fixed
Copy link

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
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
156 checks passed
@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.

2 participants