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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

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
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.

2 participants