Page MenuHomePhabricator

InstCombine: Allow sinking instructions with more uses in the same block.
Needs ReviewPublic

Authored by iteratee on May 10 2017, 2:56 PM.



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

Diff Detail

Event Timeline

iteratee created this revision.May 10 2017, 2:56 PM



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.


Maybe clearer if you turn this into an early return?

if (!UsesInOneBlock)
  return false;

// rest of code.
iteratee added inline comments.May 11 2017, 7:41 AM

I think it's conceptually clearer with the else. First run vs following runs. It's probably correct either way.


Clearer, but incorrect, there are additional things below.

iteratee updated this revision to Diff 98688.May 11 2017, 3:23 PM

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.

sanjoy added a subscriber: sanjoy.May 14 2017, 1:50 PM

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.