fix[venom]: fix duplicate allocas

charles-cooper commented 1 month ago

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-->]()

harkal commented 1 month ago

CurveStableSwapNG-0.4.1.vy fails to compile with these changes

cyberthirst commented 1 week ago

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 commented 6 days ago

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

charles-cooper commented 6 days ago

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

vyperlang / vyper