fix[venom]: track multi-output phi origins by variable#5091
Open
banteg wants to merge 5 commits into
Open
Conversation
HodanPlodky
reviewed
Jun 17, 2026
| _check_pre_post(pre, post, hevm=True) | ||
|
|
||
|
|
||
| def test_phi_elim_multi_output_invoke(): |
Collaborator
There was a problem hiding this comment.
this test does not fail on master, there was a fix for similar issue in the dyn alloc PR
Contributor
Author
There was a problem hiding this comment.
Thanks, yes, the #4922 dyn-alloc merge added the conservative bailout, so the old distinct-output test no longer fails on current master.
I merged current master into this branch, removed that now-redundant bailout now that origins carry the produced variable, and added test_phi_elim_multi_output_invoke_same_output_eliminated. That new case distinguishes current master: the same shape leaves phis_after_pass=1 there, while this branch eliminates it. I kept the distinct-output safety test as coverage for #5039 and updated the PR title/body to describe the post-#4922 scope.
Validation:
uv run --python 3.12 -m pytest tests/unit/compiler/venom/test_phi_elimination.py -quv run --python 3.12 black --check vyper/venom/passes/phi_elimination.py tests/unit/compiler/venom/test_phi_elimination.pygit diff --check
banteg
changed the title
HodanPlodky
reviewed
Jun 17, 2026
| # are keyed by the produced variable and not just the instruction, | ||
| # since different outputs of one multi-output instruction (e.g. | ||
| # invoke) are distinct origins. | ||
| phi_to_origins: dict[IRInstruction, set[tuple[IRInstruction, IROperand]]] |
Collaborator
There was a problem hiding this comment.
I think it would be better to only keep account of operands since operand identifies the inst
Contributor
Author
There was a problem hiding this comment.
Done. phi_to_origins is now set[IROperand]: the root operand carries both producer identity and output slot identity, so we no longer store (inst, operand) pairs. Real multi-origin phis use their own output operand as the barrier marker, and _process_phi assigns from the sole root operand unless it is the phi output itself.
Validation:
uv run --python 3.12 -m pytest tests/unit/compiler/venom/test_phi_elimination.py -quv run --python 3.12 black --check vyper/venom/passes/phi_elimination.py tests/unit/compiler/venom/test_phi_elimination.pygit diff --check
I also updated the PR body to describe the operand-only representation.
Gas ChangesNo changes detected. Summary
|
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
HodanPlodky
approved these changes
Jun 18, 2026
HodanPlodky
approved these changes
Jun 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
Fixes #5039 by making
PhiEliminationPassdistinguish outputs of the same multi-output producer.Current master already avoids the original #5039 crash and
python -Omiscompile via the conservative multi-output bailout added in #4922. This PR keeps the distinct-output safety, but replaces the bailout with precise origin tracking so trivial phis over the same output slot can still be eliminated.How I did it
Phi origins are now tracked by root operand. The operand identifies both its producer and the produced output slot, so
%r1and%r2from the sameinvokeremain distinct origins without storing an extra producer-instruction key. When all paths reach the same output operand,_process_phiassigns from that operand. When a phi is a real multi-origin join, its own output operand remains the barrier origin.How to verify it
test_phi_elim_multi_output_invoke_distinct_outputs_keptcovers the #5039 safety case: a phi over two different outputs of the sameinvokesurvives.test_phi_elim_multi_output_invoke_same_output_eliminatedcovers the master-distinguishing case: a phi over the same output variable of a multi-outputinvokeis eliminated. On current master, the same shape leavesphis_after_pass=1; this branch eliminates it.Validation run:
Commit message
Description for the changelog
Fix: Venom phi elimination now tracks multi-output origins by produced operand, keeping distinct-output phis while still eliminating trivial same-output phis.