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 duplicate allocas #4321

Merged
merged 25 commits into from
Nov 26, 2024

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Oct 21, 2024

What I did

How I did it

How to verify it

Commit message

this commit fixes a bug in the ir_node_to_venom translator. previously,
`ir_node_to_venom` tried to detect unique allocas based on
heuristics. this commit removes the heuristics and fixes the issue
in the frontend by passing through a unique ID for each variable in
the metadata. this ID is also passed into the `alloca` and `palloca`
instructions to aid with debugging. note that this results in improved
code, presumably due to more allocas being able to be reified.

this commit makes a minor change to the `sqrt()`, builtin, which is to
use `z_var.as_ir_node()` instead of `z_var.pos`, since `.as_ir_node()`
correctly tags with the alloca metadata. to be maximally conservative,
we could branch, only using `z_var.as_ir_node()` if we are using
the venom pipeline, but the change should be correct for the legacy
pipeline as well anyways.

Description for the changelog

Cute Animal Picture

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

@charles-cooper charles-cooper changed the title remove global symbol copying fix[venom]: fix duplicate allocas Oct 21, 2024
@harkal
Copy link
Collaborator

harkal commented Oct 23, 2024

CurveStableSwapNG-0.4.1.vy fails to compile with these changes

@charles-cooper charles-cooper marked this pull request as ready for review November 22, 2024 13:35
@cyberthirst
Copy link
Collaborator

apart from the misleading comment lgtm. I agree with the commit msg in that it shouldn't be necessary to introduce venom-only path. checked only the legacy version.

@HodanPlodky
Copy link
Collaborator

There seems to be small regression on VaultV3-0.4.0.vy codesize other then that it LGTM

@charles-cooper
Copy link
Member Author

There seems to be small regression on VaultV3-0.4.0.vy codesize other then that it LGTM

i'm guessing it breaks some alias case in mem2var.. but we should get this in for correctness and work separately on mem2var

@charles-cooper charles-cooper enabled auto-merge (squash) November 26, 2024 11:27
@charles-cooper charles-cooper merged commit cda634d into vyperlang:master Nov 26, 2024
155 checks passed
@charles-cooper charles-cooper deleted the fix/alloca branch November 26, 2024 11:45
Copy link

codecov bot commented Feb 24, 2025

Codecov Report

Attention: Patch coverage is 25.58140% with 32 lines in your changes missing coverage. Please review.

Project coverage is 52.70%. Comparing base (f249c93) to head (e80f23a).
Report is 60 commits behind head on master.

Files with missing lines Patch % Lines
vyper/venom/ir_node_to_venom.py 7.14% 13 Missing ⚠️
vyper/venom/passes/float_allocas.py 18.75% 13 Missing ⚠️
vyper/codegen/context.py 42.85% 4 Missing ⚠️
vyper/venom/__init__.py 0.00% 1 Missing ⚠️
vyper/venom/passes/sccp/sccp.py 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (f249c93) and HEAD (e80f23a). Click for more details.

HEAD has 243 uploads less than BASE
Flag BASE (f249c93) HEAD (e80f23a)
258 15
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4321       +/-   ##
===========================================
- Coverage   91.04%   52.70%   -38.35%     
===========================================
  Files         112      113        +1     
  Lines       16043    16061       +18     
  Branches     2698     2703        +5     
===========================================
- Hits        14607     8465     -6142     
- Misses       1001     6955     +5954     
- Partials      435      641      +206     

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants