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.

Thanks for the review. I have just pushed the patch. Feel free to suggest ideas to extend this patch and I will be happy to work on them.

In D55882#1336102, @arsenm wrote:

LGTM. Can the scalar load intrinsics also be handled?