[Inline Spiller] Consider bundles when marking defs as dead

[piotr]

Summary

Fix bug where the code expects just a single MI, but a series of
bundled MIs need to be handled instead.

The semi-formed bundled are created by SplitKit for the case where
not all lanes are live (buildSingleSubRegCopy). Then the remat
kicks in, and since the values that are copied in the bundle do not
need to be preserved due to the remat (dead defs), all instructions
in the bundle should be marked as dead.

However, only the first one gets marked as dead, which causes the
verifier to complain later with error: "Live range continues after
dead def flag".

[piotr]

The test case is dependent on AMDGPU-specific D154083, which rematerializes instructions with wide registers. I was not able to observe the erroneous behaviour without it, but should be possible to trigger it somehow.

[piotr]

I am open to suggestions how to handle this more gracefully.

[arsenm]

llvm/lib/CodeGen/InlineSpiller.cpp

762

I don't think you can necessarily just delete any dead defs within the bundle without knowing you can delete all the dead defs in the bundle (with the possible exception of copies)

765

this is just ++It

[arsenm]

llvm/lib/CodeGen/InlineSpiller.cpp

759

I also think addRegisterDead only needs to be done for one instruction in the bundle?

[piotr]

llvm/lib/CodeGen/InlineSpiller.cpp

762

How can I know if it is safe to delete the dead defs in the bundle then? Do I need a stricter check somewhere? Should this be checking for the COPY explicitly?

[piotr]

llvm/lib/CodeGen/InlineSpiller.cpp

762

Are you suggesting this should just go through the defs first and see if all of them can be deleted, and only then do the deletion?

[arsenm]

llvm/lib/CodeGen/InlineSpiller.cpp

762

Yes, with the possible exception of copies. I'm unclear on the rules since these aren't fully formed bundles.

[arsenm]

Don't think you can just rip out instructions in the middle of a bundle (except maybe for copy)

[piotr]

llvm/lib/CodeGen/InlineSpiller.cpp

762

The verifier error I'm chasing here only happens for copies, as it is hitting copies produced in buildSingleSubRegCopy, so special-casing copies would work here. I'll post an update soon.

[piotr]

Reworked to check for copy.

[arsenm]

llvm/lib/CodeGen/InlineSpiller.cpp

764

Think you need to check isCopyInstr, not the lower level isCopy

768

My point was you should delete the whole bundle at a time, not just prune the copies out of it

[piotr]

llvm/lib/CodeGen/InlineSpiller.cpp

768

Ok, I see. I thought copies are safe to just delete. Will make the change.

[piotr]

Delete only if all copies are dead, use isCopyInstr.

[arsenm]

llvm/lib/CodeGen/InlineSpiller.cpp

768

You need to check the outputs from isCopyInstr, it's not a boolean and you can't expect the operand indexes

[piotr]

Corrected the use of isCopyInstr.

llvm/lib/CodeGen/InlineSpiller.cpp

for (VNInfo *VNI : LI.vnis()) {
       if (VNI->isUnused() || VNI->isPHIDef() || UsedValues.count(VNI))
         continue;
       MachineInstr *MI = LIS.getInstructionFromIndex(VNI->def);
       MI->addRegisterDead(Reg, &TRI);
       if (!MI->allDefsAreDead())
         continue;
       LLVM_DEBUG(dbgs() << "All defs dead: " << *MI);
       DeadDefs.push_back(MI);
+      // If MI is a bundle header, also try removing copies inside the bundle,
+      // otherwise the verifier would complain "live range continues after dead
+      // def flag".
+      if (MI->isBundledWithSucc() && !MI->isBundledWithPred()) {
+        MachineBasicBlock::instr_iterator BeginIt = MI->getIterator(),
+                                          EndIt = MI->getParent()->instr_end();
+        ++BeginIt; // Skip MI that was already handled.
+
+        bool OnlyDeadCopies = true;
+        for (MachineBasicBlock::instr_iterator It = BeginIt;
+             It != EndIt && It->isBundledWithPred(); ++It) {
+
+          auto DestSrc = TII.isCopyInstr(*It);
+          bool IsCopyToDeadReg =
+              DestSrc && DestSrc->Destination->getReg() == Reg;
+          if (!IsCopyToDeadReg) {
+            OnlyDeadCopies = false;
+            break;
+          }
+        }
+        if (OnlyDeadCopies) {
+          for (MachineBasicBlock::instr_iterator It = BeginIt;
+               It != EndIt && It->isBundledWithPred(); ++It) {
+            It->addRegisterDead(Reg, &TRI);
+            LLVM_DEBUG(dbgs() << "All defs dead: " << *It);
+            DeadDefs.push_back(&*It);
+          }
+        }
+      }
     }
   }
 
   // Eliminate dead code after remat. Note that some snippet copies may be
   // deleted here.
   if (DeadDefs.empty())
     return;
   LLVM_DEBUG(dbgs() << "Remat created " << DeadDefs.size() << " dead defs.\n");

llvm/test/CodeGen/AMDGPU/dead_bundle.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -mtriple=amdgcn--amdpal -march=amdgcn -mcpu=gfx1100 -verify-machineinstrs=0 -start-before=greedy,0 -stop-after=virtregrewriter,0 -stress-regalloc=5 %s -o - | FileCheck %s
+# RUN: llc -mtriple=amdgcn--amdpal -march=amdgcn -mcpu=gfx1100 -verify-machineinstrs=1 -start-before=greedy,0 -stop-after=virtregrewriter,0 -stress-regalloc=5 %s -o - | FileCheck %s
 
-# This test currently fails with verify-machineinstrs=1 due to dead bundle mishandling: "Live range continues after dead def flag".
+# This test checks that dead bundles are handled correctly.
 ---
 name: psmain
 tracksRegLiveness: true
 machineFunctionInfo:
   stackPtrOffsetReg: '$sgpr32'
   psInputAddr:     7
   psInputEnable:   7
 body:             |
   bb.0:
     liveins: $sgpr0, $sgpr1, $sgpr2, $sgpr3, $vgpr0, $vgpr1, $vgpr2
 
     ; CHECK-LABEL: name: psmain
     ; CHECK: liveins: $sgpr0, $sgpr1, $sgpr2, $sgpr3, $vgpr0, $vgpr1, $vgpr2
     ; CHECK-NEXT: {{  $}}
     ; CHECK-NEXT: dead renamable $sgpr3 = IMPLICIT_DEF
     ; CHECK-NEXT: renamable $sgpr1 = KILL undef $sgpr1
     ; CHECK-NEXT: renamable $sgpr4_sgpr5_sgpr6_sgpr7_sgpr8_sgpr9_sgpr10_sgpr11 = S_BUFFER_LOAD_DWORDX8_IMM undef renamable $sgpr0_sgpr1_sgpr2_sgpr3, 416, 0 :: (dereferenceable invariant load (s256), align 4)
     ; CHECK-NEXT: dead [[V_CVT_U32_F32_e64_:%[0-9]+]]:vgpr_32 = V_CVT_U32_F32_e64 0, $sgpr4, 0, 0, implicit $mode, implicit $exec
-    ; CHECK-NEXT: SI_SPILL_S256_SAVE renamable $sgpr4_sgpr5_sgpr6_sgpr7_sgpr8_sgpr9_sgpr10_sgpr11, %stack.0, implicit $exec, implicit $sgpr32 :: (store (s256) into %stack.0, align 4, addrspace 5)
     ; CHECK-NEXT: dead renamable $sgpr4_sgpr5_sgpr6_sgpr7_sgpr8_sgpr9_sgpr10_sgpr11_sgpr12_sgpr13_sgpr14_sgpr15_sgpr16_sgpr17_sgpr18_sgpr19 = IMPLICIT_DEF
     ; CHECK-NEXT: renamable $sgpr4_sgpr5_sgpr6_sgpr7_sgpr8_sgpr9_sgpr10_sgpr11 = S_BUFFER_LOAD_DWORDX8_IMM undef renamable $sgpr0_sgpr1_sgpr2_sgpr3, 416, 0 :: (dereferenceable invariant load (s256), align 4)
     ; CHECK-NEXT: renamable $sgpr3 = COPY killed renamable $sgpr7
     ; CHECK-NEXT: renamable $sgpr5 = COPY renamable $sgpr9
     ; CHECK-NEXT: dead undef %4.sub0:vreg_64 = COPY renamable $sgpr3
     ; CHECK-NEXT: dead undef %7.sub1:vreg_64 = COPY killed renamable $sgpr5
     ; CHECK-NEXT: dead [[IMAGE_SAMPLE_V1_V2_gfx11_:%[0-9]+]]:vgpr_32 = IMAGE_SAMPLE_V1_V2_gfx11 undef %4, undef renamable $sgpr12_sgpr13_sgpr14_sgpr15_sgpr16_sgpr17_sgpr18_sgpr19, renamable $sgpr0_sgpr1_sgpr2_sgpr3, 1, 1, 0, 0, 0, 0, 0, 0, 0, implicit $exec :: (dereferenceable load (s32), addrspace 8)
     ; CHECK-NEXT: S_ENDPGM 0