r364472

ping

r364470

LGTM, but I really don't like how this keeps getting put off

Looks like this was r364342

LGTM

In D63825#1559365, @thegameg wrote:

Any way we can get a test case for this? Other than that, LGTM.

Needs test

r364425

ping

Fix selection

Diff looks wrong and is missing test

LGTM

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;
}

In D63709#1557646, @arsenm wrote:

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

Other targets seem to run SimiplifyCFG after AtomicExpand, which we are missing. Even with that disabled and the phi survives to machineinstrs, aarch64 and hexagon both avoid this

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

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

In D63709#1557592, @arsenm wrote:

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

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

I'm still not convinced just sticking EXEC reads on copies like this is a complete solution. Yes, we'll now insert the exec read on any copies that came out of isel, but any pass could introduce new copies. I guess if generic passes were more restricted when copies have implicit uses? It would require careful auditing of any generic code that can introduce copies, and may be more pessimizing than we really want

r364308

r364304

LGTM

LGTM

r364235

r364233

r364234

In D63712#1555460, @tpr wrote:

I don't think anyone is a fan of dumpcode. But we're still in the position that the proper disassembler does not support gfx6 or gfx7, and we need to get this particular problem fixed in the short term.

ping

r364215

r364212

I do suspect the handling of the EXEC physreg use in MachineCSE. I don't see any of the typical sources of late visible constants in this testcase?

In D63709#1555766, @piotr wrote:

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.

Fix test diff

Remove tests that belong with the next patch

ping