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

vyper/venom/function.py Outdated Show resolved Hide resolved
@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
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