Changed "== 16" to "!= 32".

Rebased and addressed review comments.

Fixed wrong spacing.

Rebased after D68384 has been merged.

A recent patch in SILoadOptimizer (D65496) conflicted with this patch. I have rebased it to the latest master, but for clarity I will split the review into two parts: a separate review for the NFC refactoring (D68384) and the current review only for merging MIMG instruction.

Addressed review comments and rebased.

In D67345#1688379, @nhaehnle wrote:

This is a useful change, but there is an unfortunate asymmetry here in how the code is structured: in addition to extractelement, we could also have shufflevector users (or masked stores etc.). Presumably we'd be able to handle all of those together without duplicating the code. Is there a way to take this into account?

Also, two comments inline.

In D68200#1688092, @arsenm wrote:

I thought this was a property of the resource descriptor? Why do you need to add it to the intrinsic?

Ping

Ping

Rebased and added missing 'const' in SILoadStoreOptimizer::dmasksCanBeCombined.

Reverting the patch and leaving the test only as it passes without the change in code.

Thank you for bearing with me, I have been away.

Ping

Ping

I think a generic pass would not be suited for our image instructions, due to the dmask special treatment. I had decided to extend the si-load-store-opt pass, because similar transformations were already handled there.

V2: Addressed review comments.

V5: Refactored code.

V4: Simplified the condition and test.

In D63860#1576545, @anton-afanasyev wrote:

In D63860#1574717, @piotr wrote:

In D63860#1572882, @anton-afanasyev wrote:

Hi @piotr, have you tried regression benchmarking with this patch? For instance, test suite.

Hi Anton, Yes, I have just run regression testing on the test-suite and the tests pass ("Expected Passes : 917").

Hmm, I'm talking about performance benchmarking, from llvm Test Suite: https://llvm.org/docs/TestingGuide.html#test-suite.

In D63860#1572882, @anton-afanasyev wrote:

Hi @piotr, have you tried regression benchmarking with this patch? For instance, test suite.

V3: Updating commit message.

V2: Removing condition that BB has to contain phis only. Added a MIR test.

Related to D63709.

In D63709#1557689, @arsenm wrote:

I think things are going from from this heuristic:

// Heuristics #3: If the common subexpression is used by PHIs, do not reuse
// it unless the defined value is already used in the BB of the new use.
bool HasPHI = false;
for (MachineInstr &UseMI : MRI->use_nodbg_instructions(CSReg)) {
  HasPHI |= UseMI.isPHI();
  if (UseMI.getParent() == MI->getParent())
    return true;
}

The second phi use is in a flow block with only the phi, so the use is in the fall through successor. Maybe it could be relaxed to look through trivial successors, at least for blocks with only phis?

In D63709#1557626, @arsenm wrote:

In D63709#1557605, @arsenm wrote:

In D63709#1557592, @arsenm wrote:

Reduced further: https://paste.debian.net/1089194

Other targets seem to not have this problem with a slightly generalized version, so I would look into how this is cleaned up there

It seems we're missing a simplifycfg run somewhere, so maybe we're thinking of this on the wrong level entirely.. If I run simplify cfg on any of the testcase variants, this problem disappears

Simplifying IR test case even more with bugpoint.

Adding IR test case example.

This opt opportunity presents itself after the register coalescer, where inline constants are moved to the same registers (attaching IR example testcase imminently). MachineCSE is run too early to spot this and the last occurence of si-shrink-instructions is run at the right time. I think, for similar reasons some other peepholes were also placed in this pass, even though they are not about 32-bit encoding.

The patch has been created for consistency in the same way as https://reviews.llvm.org/D49483, where a similar optimisation was added.

@Carrot Thanks for letting me know.

OK, thanks!

Yes, we see improvements on the AMDGPU target.

It's indeed unfortunate that you saw a regression with my patch. The patch makes SelectionDAG see more context, so it provides more optimization opportunities. Arguably, the fact that you see worse code now is a limitation of the optimization you mentioned that has become visible only now. Have you checked whether the problematic opt could be restricted (possibly only on targets not benefiting from the broader scope of the SelectionDAG)?

@RKSimon No, I do not intend to modify ComputeNumSignBits at this time.

Ping.

Rebased.

Addressed review comments, extracted new tests and rebased.