Rebase after D88225.

In D88245#2293131, @arsenm wrote:

LGTM (although I think readfirstlane is the same cost as a regular copy)

Looks good to me but I guess it needs approval from the affected target maintainers.

I have tried several variations on this:

# RUN: llc -march=amdgcn -run-pass=greedy,virtregrewriter -verify-regalloc < %s | FileCheck %s

In D88020#2291428, @kparzysz wrote:

I don't think it's safe to add dead defs to the extra lanes. You could split the superset into the matching part and the remaining part, then only operate on the matching one.

In D88028#2285683, @rampitec wrote:

Can this appear later in the codegen?

Add a move to materialize the constant.

The only codegen diffs I saw across several thousand graphics shaders were three instances of:

-	s_pack_ll_b32_b16 s3, s2, 0
+	s_and_b32 s3, 0xffff, s2

In the test case, RAGreedy::tryLocalSplit splits %72 at 7376r. First we have this:

%72 [1072r,7376r:0)[7376r,7440r:1)  0@1072r 1@7376r L000000000000000C [1072r,1072d:0)[7376r,7440r:1)  0@1072r 1@7376r L00000000000000F3 [1072r,7440r:0)  0@1072r weight:5.969267e-04
...
1072B	  %72:sgpr_128 = COPY %71:sgpr_128
...
7376B	  %72.sub1:sgpr_128 = COPY %912:sreg_32_xm0_xexec
7440B	  %558:sreg_32_xm0_xexec = S_BUFFER_LOAD_DWORD_IMM %72:sgpr_128, 0, 0, 0 :: (dereferenceable invariant load 4)

Then SplitEditor::enterIntvBefore creates %914 and inserts the bundled copies (annoying the SlotIndexes get renumbered at this point):

%72 [1072r,7388r:0)[7388r,7452r:1)  0@1072r 1@7388r L000000000000000C [1072r,1072d:0)[7388r,7452r:1)  0@1072r 1@7388r L00000000000000F3 [1072r,7452r:0)  0@1072r weight:5.969267e-04
%914 EMPTY  0@7380r L00000000000000F0 [7380r,7380d:0)  0@7380r L0000000000000003 [7380r,7380d:0)  0@7380r L000000000000000C EMPTY weight:0.000000e+00
...
1072B	  %72:sgpr_128 = COPY %71:sgpr_128
...
7380B	  undef %914.sub2_sub3:sgpr_128 = COPY %72.sub2_sub3:sgpr_128 {
	    internal %914.sub0:sgpr_128 = COPY %72.sub0:sgpr_128
7388B	  }
  %72.sub1:sgpr_128 = COPY %912:sreg_32_xm0_xexec
7452B	  %558:sreg_32_xm0_xexec = S_BUFFER_LOAD_DWORD_IMM %72:sgpr_128, 0, 0, 0 :: (dereferenceable invariant load 4)

Then SplitEditor::finish tries to add dead defs for %914 L00000000000000F0 and L0000000000000003 but fails to find matching lane masks in the original live ranges for %72 -- which just had a single lane mask L00000000000000F3.

In D87502#2267572, @foad wrote:

This isn't ready to commit yet:

1. there are a few code quality regressions in the test cases
2. it provokes a crash on test/CodeGen/AMDGPU/global-extload-i16.ll which I haven't been able to fix yet

Rebase.

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.

Generate test checks for splitkit-copy-bundle.mir.

Factor instruction definitions into a class.

Generate test checks.

In D87757#2276571, @arsenm wrote:

I'm assuming this doesn't fix the problem of spilling unnecessary lanes?

@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

In D87585#2276099, @critson wrote:

In D87585#2276097, @foad wrote:

Are there any other significant changes? I'm thinking of things like this that would help with dependency stalls on gfx10:

v_mov v0, 0
v_mov v1, v0

->

v_mov v0, 0
v_mov v1, 0

In D87585#2274305, @rampitec wrote:

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.

The encoding seems OK. I can pre-commit the -show-mc-encoding change to make that clearer if you want.

Add -show-mc-encoding.

With this patch applied I'm getting:

$ ~/llvm-debug/bin/llc -march=amdgcn -o /dev/null -regalloc=fast x.ll
llc: /home/jayfoad2/git/llvm-project/llvm/lib/CodeGen/RegAllocFast.cpp:876: void (anonymous namespace)::RegAllocFast::useVirtReg(llvm::MachineInstr &, unsigned int, llvm::Register): Assertion `!MO.isTied() && "tied op should be allocated"' failed.

In D87585#2271185, @arsenm wrote:

Where/why is renameable set in the first place? Can we just avoid setting it to begin with?

Do you have any stats on how many more copies get propagated, and how many instructions that actually saves in the final ISA? I have a feeling that SIFoldOperands currently catches a lot of the cases that MachineCopyProp misses.

This will be fixed by in D87511.

In D87502#2267572, @foad wrote:

2. it provokes a crash on test/CodeGen/AMDGPU/global-extload-i16.ll which I haven't been able to fix yet

This isn't ready to commit yet:

1. there are a few code quality regressions in the test cases
2. it provokes a crash on test/CodeGen/AMDGPU/global-extload-i16.ll which I haven't been able to fix yet

In D87464#2267330, @nikic wrote:

In D87464#2265954, @foad wrote:

@nikic this could perhaps increase compile time. Could you test it please?

Not seeing any impact from this change (https://llvm-compile-time-tracker.com/compare.php?from=a5168bdb4a25485ac62e18bdc538b4842bc9fbd9&to=c23aa3c1c163fc5343b1df5254b8c0782d4324cb&stat=instructions).

Rename the pseudo-instructions.

@nikic this could perhaps increase compile time. Could you test it please?

In D86294#2261887, @arsenm wrote:

I think copies that can be ignored should be taken care of by a combiner pass, and the selector should not have to do this

How does the new test case relate to the tablegen change? Perhaps you could pre-commit it so we can see the test case diffs?

Looks good to me if Matt has no further comments. I'm not sure whether gfx10.1.1 has the bug but it's certainly safe to assume it does, unless/until we know otherwise.

In D87034#2259745, @nikic wrote:

Contrary to my expectations, this change had a small but measurable compile-time impact: https://llvm-compile-time-tracker.com/compare.php?from=04ea680a8ccc4f9a4d7333cd712333960348c35b&to=5350e1b5096aa4707aa525baf7398d93b4a4f1a5&stat=instructions

Unfortunately, I have no idea why. I looked at callgrind profiles and the extra instructions (consistently) end up inside a DenseMap method (without changes to number of calls). Can't explain why that would happen, and the diff is not large enough to bother looking further.

I did end up applying this small cleanup while looking over the code again: https://github.com/llvm/llvm-project/commit/ddab4cd83ea31141aaada424dccf94278482ee88

Typo in summary "except".

N.B without D87034, this change wouldn't affect any codegen tests. So this is one case where the improved known bits analysis actually makes a difference.

Simplify makeGE.

Use it in SelectionDAG, GlobalISel and ValueTracking.