SSA-CFG: Fix detection of equivalent instructions in Outliner#16768
SSA-CFG: Fix detection of equivalent instructions in Outliner#16768blishko wants to merge 1 commit into
Conversation
In the helper we use to check if arguments of two instructions are equivalent, we would say that two instructions are equivalent if they have the same index. This is, however, not correct, as the two instructions can come from two different CFGs. Thus, same indices does not mean they refer to the same instruction. We fixed the check instead to - Check if we have proven equivalence before; or - The two instructions refer to the same literal
|
This pull request is stale because it has been open for 14 days with no activity. |
github-actions
Bot
added
the
stale
blishko
added
experimental
and removed
stale
clonker
left a comment
clonker
left a comment
There was a problem hiding this comment.
Your fix works but the component is still a bit broken. We should generally define tests for optimizer passes.
| if (inst1.opcode == InstOpcode::BuiltinCall && cfg1.builtinPayload(iid1).builtin != cfg2.builtinPayload(iid2).builtin) | ||
| return false; |
There was a problem hiding this comment.
Different issue here, this does not treat literal/immediate arguments of the builtins properly. Eg two verbatim blocks with different bytecode would be deduplicated. We should add something like
if (inst1.opcode == InstOpcode::BuiltinCall && !sameLiteralArguments(cfg1.builtinPayload(iid1).literalArguments, cfg2.builtinPayload(iid2).literalArguments))
return false;underneath the builtin payload check. Or have a helper function that compares builtins and use that for both: compare builtin handle and, for literal args, Literal::kind and Literal::value.
| } | ||
| case InstOpcode::Projection: | ||
| { | ||
| instMapping.insert({iid1.value, iid2.value}); |
There was a problem hiding this comment.
This relies on the implicit invariant that the nth projection on cfg1 pairs with the nth projection on cfg2. We could harden it a bit:
| yulAssert(cfg1.projectionIndex(iid1) == cfg2.projectionIndex(iid2)); | |
| instMapping.insert({iid1.value, iid2.value}); |
|
It seems to me this needs some more thought. |
2 participants
In the helper we use to check if arguments of two instructions are equivalent, we would say that two instructions are equivalent if they have the same index.
This is, however, not correct, as the two instructions can come from two different CFGs. Thus, same indices does not mean they refer to the same instruction.
We fixed the check instead to