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]: liveness analysis in some loops #3732

Merged

Conversation

harkal
Copy link
Collaborator

@harkal harkal commented Jan 17, 2024

What I did

Fixed a small issue with the calculation of liveness in the case of variables in simple loops.

How I did it

Refactored the liveness calculation algorithm to an iterative version over the recursive one. Along side some additional refactoring to make the code cleaner and easier to follow.

How to verify it

I added the test_liveness_simple_loop.py that failed without the fix

Commit message

fixes a small issue with the calculation of liveness when variables are
consumed inside of loops by refactoring the liveness calculation
algorithm to an iterative version over the recursive one

additional QOL refactoring:
- simplify convert_ir_basicblock - rename it to ir_node_to_venom
- fix bb well-formedness for deploy ir
- don't do deploy IR detection; rely on caller in CompilerData to call for
  both deploy and runtime IR
- remove findIRnode, it's no longer needed
- rename _convert_ir_basicblock to _convert_ir_bb
- add _convert_ir_bb_list helper to handle arglists

Description for the changelog

Cute Animal Picture

Super liveness animal

@harkal harkal changed the title [bugfix] Liveness fix simple loop case [bugfix][venom] Liveness fix simple loop case Jan 17, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2024

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (81c6d8e) 84.18% compared to head (cc4c174) 84.86%.

Files Patch % Lines
vyper/venom/ir_node_to_venom.py 36.36% 40 Missing and 2 partials ⚠️
vyper/compiler/phases.py 0.00% 4 Missing ⚠️
vyper/venom/__init__.py 50.00% 2 Missing ⚠️
vyper/venom/analysis.py 85.71% 0 Missing and 2 partials ⚠️
vyper/venom/basicblock.py 0.00% 1 Missing ⚠️
vyper/venom/function.py 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3732      +/-   ##
==========================================
+ Coverage   84.18%   84.86%   +0.68%     
==========================================
  Files          92       92              
  Lines       13143    13128      -15     
  Branches     2928     2927       -1     
==========================================
+ Hits        11064    11141      +77     
+ Misses       1659     1525     -134     
- Partials      420      462      +42     

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

@charles-cooper charles-cooper added bug - venom bug in experimental venom pipeline and removed bug - codegen labels Jan 17, 2024
@harkal harkal marked this pull request as ready for review January 17, 2024 17:08
@charles-cooper charles-cooper changed the title [bugfix][venom] Liveness fix simple loop case fix[venom]: liveness analysis in some loops Jan 17, 2024
tests/unit/compiler/venom/test_liveness_simple_loop.py Outdated Show resolved Hide resolved
vyper/venom/__init__.py Outdated Show resolved Hide resolved
- rename it to ir_node_to_venom
- fix bb well-formedness for deploy ir
- don't do deploy IR detection; rely on caller in CompilerData to call for
  both deploy and runtime IR
- remove findIRnode, it's no longer needed
@charles-cooper charles-cooper enabled auto-merge (squash) January 17, 2024 21:40
@charles-cooper charles-cooper merged commit c42b077 into vyperlang:master Jan 17, 2024
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug - venom bug in experimental venom pipeline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants