[AMDGPU][SIFoldOperands] Clear kills when folding COPY

[critson]

Summary

Clear all kill flags on source register when folding a COPY.
This is necessary because the kills may now be out of order with the uses.

[foad]

So you start with this:

%2:sreg_64_xexec = COPY %1.sub0_sub1:sgpr_128
%3:sreg_64_xexec = COPY killed %1.sub2_sub3:sgpr_128
%4:sreg_64 = COPY %2:sreg_64_xexec

and folding %2 into %4 gives this:

%3:sreg_64_xexec = COPY killed %1.sub2_sub3:sgpr_128
%4:sreg_64 = COPY %1.sub0_sub1:sgpr_128

but now the killed flag on the RHS of %3 is wrong. How is this handled in the normal non-subreg case?

[foad]

Here's a test case that shows the same bug with no subregs involved:

---
name: fold_reg_kill
tracksRegLiveness: true
body: |
  bb.0:
    liveins: $sgpr0

    %0:sreg_32 = COPY $sgpr0
    %1:sreg_32 = COPY %0
    %2:sreg_32 = S_ADD_U32 killed %0, killed %0, implicit-def $scc
    %3:sreg_32 = COPY %1
    S_ENDPGM 0, implicit %2, implicit %3
...

I wonder why we never normally see this in practice?

[critson]

In D130622#3682085, @foad wrote:

I wonder why we never normally see this in practice?

Perhaps it is happening, but without the verifier enabled?

[critson]

• Generalise beyond subregister case

[foad]

I wonder why we never normally see this in practice?

I think we only go into this code if there is a COPY of the result of another COPY. Perhaps these are normally copy-propagated by some other pass before we get here?

[foad]

I wonder why we never normally see this in practice?

I think we only go into this code if there is a COPY of the result of another COPY. Perhaps these are normally copy-propagated by some other pass before we get here?

See: https://reviews.llvm.org/D52577#1248697 which introduced this code, and the comment: "Why is this necessary? PeepholeOpt runs first and should have eliminated redundant copies"

[foad]

Anyway the patch looks OK to me, given that this code does not fire very often. One day we should take a closer look at whether it is still needed, or whether PeepholeOptimizer could be improved instead. (Or perhaps it already has been? I see D87939 landed since this code was written.)

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

741

Don't need the braces.

[arsenm]

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

741–743

You've reinvented MRI.clearKillFlags

[critson]

In D130622#3682499, @foad wrote:

I wonder why we never normally see this in practice?

I think we only go into this code if there is a COPY of the result of another COPY. Perhaps these are normally copy-propagated by some other pass before we get here?

See: https://reviews.llvm.org/D52577#1248697 which introduced this code, and the comment: "Why is this necessary? PeepholeOpt runs first and should have eliminated redundant copies"

In my specific case these extra COPYs come from SILoadStoreOptimizer.

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

741–743

Thanks, I had missed there was a helper for this.

[foad]

See: https://reviews.llvm.org/D52577#1248697 which introduced this code, and the comment: "Why is this necessary? PeepholeOpt runs first and should have eliminated redundant copies"

In my specific case these extra COPYs come from SILoadStoreOptimizer.

Maybe SILoadStoreOptimizer should not insert copies at all. If it replaces:

%old0:vgpr_32 = BUFFER_LOAD_DWORD ...
%old1:vgpr_32 = BUFFER_LOAD_DWORD ...

with:

%new:vgpr_64 = BUFFER_LOAD_DWORDX2 ...

then perhaps it could just replace all uses of %old0 with %new.sub0?

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

if (UseMI->isCopy() && OpToFold.isReg() &&
       unsigned Size = TII->getOpSize(*UseMI, 1);
       Register UseReg = OpToFold.getReg();
       UseMI->getOperand(1).setReg(UseReg);
       UseMI->getOperand(1).setSubReg(OpToFold.getSubReg());
       UseMI->getOperand(1).setIsKill(false);
       CopiesToReplace.push_back(UseMI);
       OpToFold.setIsKill(false);
 
+      // Remove kill flags as kills may now be out of order with uses.
+      MRI->clearKillFlags(OpToFold.getReg());
+
       // That is very tricky to store a value into an AGPR. v_accvgpr_write_b32
       // can only accept VGPR or inline immediate. Recreate a reg_sequence with
       // its initializers right here, so we will rematerialize immediates and
       // avoid copies via different reg classes.
       SmallVector<std::pair<MachineOperand*, unsigned>, 32> Defs;
       if (Size > 4 && TRI->isAGPR(*MRI, UseMI->getOperand(0).getReg()) &&
           getRegSeqInit(Defs, UseReg, AMDGPU::OPERAND_REG_INLINE_C_INT32, TII,
                         *MRI)) {
         const DebugLoc &DL = UseMI->getDebugLoc();

llvm/test/CodeGen/AMDGPU/si-fold-copy-kills.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -run-pass=si-fold-operands -verify-machineinstrs -o - %s | FileCheck --check-prefix=GCN %s
+
+---
+name: fold_reg_kill
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $sgpr0
+
+    ; GCN-LABEL: name: fold_reg_kill
+    ; GCN: liveins: $sgpr0
+    ; GCN-NEXT: {{  $}}
+    ; GCN-NEXT: [[COPY:%[0-9]+]]:sreg_32 = COPY $sgpr0
+    ; GCN-NEXT: [[S_ADD_U32_:%[0-9]+]]:sreg_32 = S_ADD_U32 [[COPY]], [[COPY]], implicit-def $scc
+    ; GCN-NEXT: [[COPY1:%[0-9]+]]:sreg_32 = COPY [[COPY]]
+    ; GCN-NEXT: S_ENDPGM 0, implicit [[S_ADD_U32_]], implicit [[COPY1]]
+    %0:sreg_32 = COPY $sgpr0
+    %1:sreg_32 = COPY %0
+    %2:sreg_32 = S_ADD_U32 killed %0, killed %0, implicit-def $scc
+    %3:sreg_32 = COPY %1
+    S_ENDPGM 0, implicit %2, implicit %3
+...
+
+---
+name:            fold_subreg_kill
+tracksRegLiveness: true
+body:             |
+  ; GCN-LABEL: name: fold_subreg_kill
+  ; GCN: bb.0:
+  ; GCN-NEXT:   successors: %bb.1(0x80000000)
+  ; GCN-NEXT:   liveins: $sgpr0_sgpr1
+  ; GCN-NEXT: {{  $}}
+  ; GCN-NEXT:   [[COPY:%[0-9]+]]:sgpr_64(p4) = COPY $sgpr0_sgpr1
+  ; GCN-NEXT:   [[S_LOAD_DWORDX4_IMM:%[0-9]+]]:sgpr_128 = S_LOAD_DWORDX4_IMM [[COPY]](p4), 9, 0 :: (load (s128), align 4, addrspace 4)
+  ; GCN-NEXT:   [[COPY1:%[0-9]+]]:sreg_64_xexec = COPY [[S_LOAD_DWORDX4_IMM]].sub2_sub3
+  ; GCN-NEXT:   [[COPY2:%[0-9]+]]:sreg_64 = COPY [[S_LOAD_DWORDX4_IMM]].sub0_sub1
+  ; GCN-NEXT: {{  $}}
+  ; GCN-NEXT: bb.1:
+  ; GCN-NEXT:   [[COPY3:%[0-9]+]]:sreg_32 = COPY [[COPY2]].sub1
+  ; GCN-NEXT:   [[COPY4:%[0-9]+]]:sreg_32 = COPY [[COPY2]].sub0
+  ; GCN-NEXT:   [[COPY5:%[0-9]+]]:sreg_32 = COPY [[COPY1]].sub1
+  ; GCN-NEXT:   [[S_MOV_B32_:%[0-9]+]]:sreg_32 = S_MOV_B32 -1
+  ; GCN-NEXT:   [[REG_SEQUENCE:%[0-9]+]]:sgpr_128 = REG_SEQUENCE killed [[COPY3]], %subreg.sub0, killed [[COPY4]], %subreg.sub1, killed [[COPY5]], %subreg.sub2, killed [[S_MOV_B32_]], %subreg.sub3
+  ; GCN-NEXT:   [[DEF:%[0-9]+]]:vgpr_32 = IMPLICIT_DEF
+  ; GCN-NEXT:   BUFFER_STORE_DWORD_OFFSET [[DEF]], killed [[REG_SEQUENCE]], 0, 0, 0, 0, 0, implicit $exec :: (store (s32), addrspace 1)
+  bb.0:
+    liveins: $sgpr0_sgpr1
+
+    %0:sgpr_64(p4) = COPY $sgpr0_sgpr1
+    %1:sgpr_128 = S_LOAD_DWORDX4_IMM %0:sgpr_64(p4), 9, 0 :: (load (s128), align 4, addrspace 4)
+    %2:sreg_64_xexec = COPY %1.sub0_sub1:sgpr_128
+    %3:sreg_64_xexec = COPY killed %1.sub2_sub3:sgpr_128
+    %4:sreg_64 = COPY %2:sreg_64_xexec
+    %5:sreg_32 = COPY %3.sub1:sreg_64_xexec
+
+  bb.1:
+    %6:sreg_32 = COPY %4.sub1:sreg_64
+    %7:sreg_32 = COPY %4.sub0:sreg_64
+    %8:sreg_32 = COPY %5:sreg_32
+    %9:sreg_32 = S_MOV_B32 -1
+    %10:sgpr_128 = REG_SEQUENCE killed %6:sreg_32, %subreg.sub0, killed %7:sreg_32, %subreg.sub1, killed %8:sreg_32, %subreg.sub2, killed %9:sreg_32, %subreg.sub3
+    %11:vgpr_32 = IMPLICIT_DEF
+    BUFFER_STORE_DWORD_OFFSET %11:vgpr_32, killed %10:sgpr_128, 0, 0, 0, 0, 0, implicit $exec :: (store (s32), addrspace 1)
+...
+