LGTM

OK, thanks Alex.

LGTM

LGTM

In D87882#2287724, @piotr wrote:

bb.1:
  ..
  TO_inst   (inits m0)
  ...       (uses m0)
  FROM_inst (clobbers m0)
  ...
  S_CBRANCH_VCCZ %bb.1, undef
  S_BRANCH %bb.2

I guess I am missing something here, the dominance usually refers to blocks so it can be confusing if applied for instructions.

In this case I expect MDT.dominates(FROM_inst, TO_inst) to return false, because it is possible for a program invocation to execute TO_inst, without executing FROM_inst first (e.g. single loop iteration).

Anyway, the spirit of the code I am editing was that before my patch it was assumed that if FROM_inst comes after TO_inst in a basic block (as in the case described above) then there was no path from FROM_inst to TO_inst, so TO_inst could be removed. This is not true for loops, where FROM_inst will clober m0 used in the next iteration.

Added legalization. Code is expectedly worse though.

LGTM. Looks like Matt has no concerns about Global ISel as well.

This is obviously LGTM from the AMDGPU BE point of view, we did it ourselves.

In D87882#2284543, @piotr wrote:

In D87882#2282886, @rampitec wrote:

In D87882#2282818, @piotr wrote:

In D87882#2282278, @rampitec wrote:

Doesn't loop block self dominate?

You mean, why the condition "MDT.dominates(From, To)" returns false if From and To are in the same BB? Inside that function there is a bb dominance check if the instructions are in different blocks, but if the instructions are in the same block the code checks whether From comes before To.

The bug I am trying to fix is simplified in the test m0-in-loop-0 where before my patch SI_INIT from line 333 was incorrectly removed. Although "m0 = COPY" comes after SI_INIT in that bb, this is a loop and in the second iteration DS_WRITE would see a clobbered m0, so SI_INIT has to be preserved.

That sounds like a bug in the dominate() to me.

MDT.dominates() gives correct answer. In the failing case "From" (m0 copy, line 335 in the test) is inside the same block as "To" (SI_INIT, line 333). "From" comes after "To", and "From" does not dominate "To" (so MDT.dominates() return false as expected), but "To" is reachable from "From" from previous iteration, so "To" cannot be removed.

Can this appear later in the codegen? It also does not cover global isel, so part in the operand folding probably needs to remain in addition to patterns.

In D87882#2282818, @piotr wrote:

In D87882#2282278, @rampitec wrote:

Doesn't loop block self dominate?

You mean, why the condition "MDT.dominates(From, To)" returns false if From and To are in the same BB? Inside that function there is a bb dominance check if the instructions are in different blocks, but if the instructions are in the same block the code checks whether From comes before To.

The bug I am trying to fix is simplified in the test m0-in-loop-0 where before my patch SI_INIT from line 333 was incorrectly removed. Although "m0 = COPY" comes after SI_INIT in that bb, this is a loop and in the second iteration DS_WRITE would see a clobbered m0, so SI_INIT has to be preserved.

Doesn't loop block self dominate?

In D87864#2282012, @arsenm wrote:

In D87864#2281901, @rampitec wrote:

I think you need to drop denorm checks and move the check outside of the address space check.

But the DS atomics do respect the denormal mode

I think you need to drop denorm checks and move the check outside of the address space check.

Actually it can be done with a singe v_perm_b32.

Replaced v_pack with two instructions.

In D87782#2277399, @arsenm wrote:

Are you planning on adding the globalisel support?

In D87757#2277557, @foad wrote:

In D87757#2277232, @rampitec wrote:

In D87757#2276505, @foad wrote:

@rampitec this patch causes CodeGen/AMDGPU/splitkit-copy-bundle.mir to fail because it no longer generates any bundles. Could you help find a way to update the test case? I have pushed this patch to github in case you want to pull from there: https://github.com/jayfoad/llvm-project/tree/only-copy-live-lanes

There is very high chance this test in unfixable with your changes as these prevent the very situation where the KIILs were formed. I'd say you may just generate checks there. At the end this code snippet did not compile and asserted, so we need to make sure it compiles. Bundles themselves are just an implementation detail.

How about this? I dropped the ASM RUN line because it's awkward to use update_llc_test_checks and update_mir_test_checks in the same file.

In D87782#2277399, @arsenm wrote:

Are you planning on adding the globalisel support?

In D87757#2276505, @foad wrote:

@rampitec this patch causes CodeGen/AMDGPU/splitkit-copy-bundle.mir to fail because it no longer generates any bundles. Could you help find a way to update the test case? I have pushed this patch to github in case you want to pull from there: https://github.com/jayfoad/llvm-project/tree/only-copy-live-lanes

LGTM

LGTM

LGTM

In D87585#2274280, @critson wrote:

The numbers for this change are not vastly compelling.
I looked at 11598 game shaders and compiled these for GFX7, GFX9 and GFX10.
On GFX7, 1 shader lost 1 instruction.
On GFX9, 1 shader lost 1 instruction, but 64 shaders gained 1 instruction.
On GFX10, 1 shader lost 1 instruction, but 2 shaders gained 1 instruction.

I started this change as I am looking at moving WQM after MI scheduling, and this potentially leaves some additional copies around.
But I could addressed these with very limited special case copy elimination in the WQM pass itself.

Does anyone have an opinion on whether I should continue pushing this?

LGTM, but please also wait for Matt's review.

LGTM

Needs test.

OK, thanks!

In D87107#2259217, @alex-t wrote:

The idea is:

For the block that is queried

1. Look for it's predecessors that can pass control through the S_EXECZ/EXECNZ
2. If found one, look for exec restoring code starting the beginning of the block being queried.
3. Since exec restoring code always belong to the block prologue, search the prologue and if not found return false.

Considering your comment that exec == 0 does not matter, we'd rather search upwards before the immediate dominator block in encountered to check what we met first - exec modify or exec restore. The problem here is that XOR can be both.

LGTM

In D87107#2257000, @alex-t wrote:

In D87107#2256913, @rampitec wrote:

In D87107#2256864, @alex-t wrote:

Also I still think that disabling a whole "endif" block is an overkill.

It only is disabled if S_OR_B64 exec, ... is in the middle of the block that should never happen.

while (isBasicBlockPrologue(*J)) {
  if (IsExecRestore(&*J))
    return true;

assumes that if exec is restored in the block prologue it is valid

So practically it never happens and split is effectively only disabled in an empty block? I said it already: it does not matter that exec is zero, what matters is that it does not match. It does not matter that a block is empty as well, it is enough to split before s_or to hit the bug.

Let's forget about exec == 0...
The code snippet above looks for exec restoring instruction in the block prologue and return true if it exists. So, false positive can only happen if there is no exec restoring instruction in the beginning of the block but it is placed in the middle of the block. In this case I report the block is invalid for splitting despite it is valid to split after the exec restoring instruction in the middle of the block. Do we really have exec restored in the middle of the block?

In D87107#2256864, @alex-t wrote:

Also I still think that disabling a whole "endif" block is an overkill.

It only is disabled if S_OR_B64 exec, ... is in the middle of the block that should never happen.

while (isBasicBlockPrologue(*J)) {
  if (IsExecRestore(&*J))
    return true;

assumes that if exec is restored in the block prologue it is valid

I still do not believe the problem is specific to EXEC = 0 case. In fact the problem could occur with any EXEC value, it is sufficient to have it different from what is expected. That is not valid to insert a split into any block before exec is restored, not just if previous value was zero.

I do not think it shall be at a block level. You can allow splitting after all exec modifications are done. For example:

LGTM, thanks. I never really understood why are we doing this copy.

LGTM with a nit: please run opt -instnamer on the test before submission.

LGTM

LGTM even if we need to rework it as Matt suggests.

LGTM, but please run PSDB before submission. This stuff is quite nontrivial.

In D86634#2245537, @alex-t wrote:

The only difference is that now these redundant branch is inserted by MachineBasicBlock::updateTerminator() as Matt suggested.

Unlimited is really too aggressive. It can slow down compilation dramatically in some cases.
Also it would be nice to see a relevant test.

LGTM