Currently InstCombine will sink instructions that may be trivially sunk if they
have a single use. This patch relaxes this requirement to 4 or fewer uses, and
requires that they all be in the same basic block. The motivating case was an
instruction that was used by 2 extractvalue instructions, both in the same
block.
Details
Diff Detail
Event Timeline
Drive-by-review.
lib/Transforms/InstCombine/InstructionCombining.cpp | ||
---|---|---|
2920 | Do you actually need the else here? I suspect CurrentUserParent will never be nullptr, and UsersParent will never be nullptr here either after the preceding conditional. | |
2927 | Maybe clearer if you turn this into an early return? if (!UsesInOneBlock) return false; // rest of code. |
Now with more Test Case!
Also, moved the compatibility test up into the Use loop, so that if the first use isn't in a compatible block, we abort.
High level comment: this feels really strange to be in instcombine.
Typically, canonicalization wants things *hoisted* not *sunk*, to make the values available for redundancy elimination.
We have a late LoopSink pass already. Do we just need to generalize it to a full sinking pass? That was discussed as a desirable long-term direction in the review.
Typically, canonicalization wants things *hoisted* not *sunk*, to make the values available for redundancy elimination.
Sinking isn't really a canonicalization on its own, but we have canonicalization passes with thresholds which are affected by sinking: jump threading, loop rotation, loop unswitching, etc.
Do you actually need the else here? I suspect CurrentUserParent will never be nullptr, and UsersParent will never be nullptr here either after the preceding conditional.