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

feat[venom]: add store elimination pass #4021

Merged
merged 12 commits into from
May 14, 2024

Conversation

harkal
Copy link
Collaborator

@harkal harkal commented May 14, 2024

What I did

I added a new pass the eliminates venom store instructions. These instructions have no equivalent in the generated code, and serve the venom code temporarily. Removing them and forwarding the results to the actual uses, helps with further optimizations.

How I did it

How to verify it

Commit message

Venom uses the `store` instruction to load a variable or literal to
another variable. This commit adds a new pass `StoreElimination` that
forwards the rhs for `store`s to their uses, eliminating the `store`
instruction in the process.

Description for the changelog

Cute Animal Picture

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

@harkal harkal changed the title feat[venom]: Add the store elimination pass feat[venom]: add the store elimination pass May 14, 2024
@harkal harkal marked this pull request as ready for review May 14, 2024 12:12
@charles-cooper charles-cooper changed the title feat[venom]: add the store elimination pass feat[venom]: add store elimination pass May 14, 2024
Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like duplication (or evolution) of

def _propagate_variables(self):
"""
Copy elimination. #NOTE: Not working yet, but it's also not needed atm.
"""
for bb in self.dom.dfs_walk:
for inst in bb.instructions:
if inst.opcode == "store":
uses = self._get_uses(inst.output)
remove_inst = True
for usage_inst in uses:
if usage_inst.opcode == "phi":
remove_inst = False
continue
for i, op in enumerate(usage_inst.operands):
if op == inst.output:
usage_inst.operands[i] = inst.operands[0]
if remove_inst:
inst.opcode = "nop"
inst.operands = []
. maybe that is dead code, and it can be removed now?

@charles-cooper
Copy link
Member

just checked and this looks like it reduces instructions by 1.6% and 1.9% on CurveStableSwapNG.vy and CurveCryptoMathOptimized.vy, respectively

@charles-cooper charles-cooper enabled auto-merge (squash) May 14, 2024 19:47
@charles-cooper charles-cooper merged commit c276c94 into vyperlang:master May 14, 2024
152 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants