AMDGPU: Fix dbg_value handling when forming soft clause bundles

[arsenm]

Summary

DBG_VALUES placed between memory instructions would change
codegen. Skip over these and re-insert them after the bundle instead
of giving up on bundling.

[arsenm]

Forgot to git add test

[dfukalov]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

388

Why don't just create DbgInstrs here?

llvm/test/CodeGen/AMDGPU/soft-clause-dbg-value.mir

3

Am I right that we need phi-node-elimination, before to remove IsSSA property to avoid verifier' fail?
Or, dummy copy of first instruction %0:sreg_64 = COPY $sgpr4_sgpr5 ; defeat IsSSA detection can be added.

[scott.linder]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

326

Since https://reviews.llvm.org/D92522, if you don't have a strong reason for choosing 8

390–392

When does Next != BundleNext after this for terminates? It seems like you should be able to just use Next below when re-inserting the DBG instructions?

[rampitec]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

388

Right, it can be local to this block and cleared/destroyed after re-insertion. It will take less memory.

[arsenm]

Address comments

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

388

This avoids reallocating on subsequent bundles if it's necessary

390–392

It doesn't make a difference

llvm/test/CodeGen/AMDGPU/soft-clause-dbg-value.mir

3

No. The pass cleared properties now clears SSA

[arsenm]

41877b82f07224041a2a994f9032332fe01e4d1b

[scott.linder]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

390–392

I disagree that it doesn't matter, after reading to this point I have to assume either (a) I don't understand the code, or (b) there is a missing assert(BundleNext == Next);

Can you either delete BundleNext or add an assert?

[arsenm]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

390–392

BundleNext is needed for incrementing the inner loop up to Next

[scott.linder]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

388

I didn't notice this when I first read, but it doesn't really seem necessary to buffer up the debug instrs at all. Can't we just MBB.insert(Next, BI->removeFromParent()) below? Did you do the buffering and extra loop because it makes tracking when to end iteration simpler, and because the largest the vector can get is amdgpu-max-memory-clause anyway?

390–392

Thank you for changing this in the committed version!

llvm/lib/Target/AMDGPU/GCNRegPressure.cpp

bool GCNDownwardRPTracker::advanceBeforeNext() {
 
   MaxPressure = max(MaxPressure, CurPressure);
 
   return true;
 }
 
 void GCNDownwardRPTracker::advanceToNext() {
   LastTrackedMI = &*NextMI++;
+  NextMI = skipDebugInstructionsForward(NextMI, MBBEnd);
 
   // Add new registers or mask bits.
   for (const auto &MO : LastTrackedMI->operands()) {
     if (!MO.isReg() || !MO.isDef())
       continue;
     Register Reg = MO.getReg();
     if (!Reg.isVirtual())
       continue;

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

 
 static bool isSMEMClauseInst(const MachineInstr &MI) {
   return SIInstrInfo::isSMRD(MI);
 }
 
 // There no sense to create store clauses, they do not define anything,
 // thus there is nothing to set early-clobber.
 static bool isValidClauseInst(const MachineInstr &MI, bool IsVMEMClause) {
-  if (MI.isDebugValue() || MI.isBundled())
+  assert(!MI.isDebugInstr() && "debug instructions should not reach here");
+  if (MI.isBundled())
     return false;
   if (!MI.mayLoad() || MI.mayStore())
     return false;
   if (AMDGPU::getAtomicNoRetOp(MI.getOpcode()) != -1 ||
       AMDGPU::getAtomicRetOp(MI.getOpcode()) != -1)
     return false;
   if (IsVMEMClause && !isVMEMClauseInst(MI))
     return false;
bool SIFormMemoryClauses::runOnMachineFunction(MachineFunction &MF) {
   SlotIndexes *Ind = LIS->getSlotIndexes();
   bool Changed = false;
 
   MaxVGPRs = TRI->getAllocatableSet(MF, &AMDGPU::VGPR_32RegClass).count();
   MaxSGPRs = TRI->getAllocatableSet(MF, &AMDGPU::SGPR_32RegClass).count();
   unsigned FuncMaxClause = AMDGPU::getIntegerAttribute(
       MF.getFunction(), "amdgpu-max-memory-clause", MaxClause);
 
+  SmallVector<MachineInstr *> DbgInstrs;
+
   for (MachineBasicBlock &MBB : MF) {
     GCNDownwardRPTracker RPT(*LIS);
     MachineBasicBlock::instr_iterator Next;
     for (auto I = MBB.instr_begin(), E = MBB.instr_end(); I != E; I = Next) {
       MachineInstr &MI = *I;
       Next = std::next(I);
 
+      if (MI.isDebugInstr())
+        continue;
+
       bool IsVMEM = isVMEMClauseInst(MI);
 
       if (!isValidClauseInst(MI, IsVMEM))
         continue;
 
       if (!RPT.getNext().isValid())
         RPT.reset(MI);
       else { // Advance the state to the current MI.
         RPT.advance(MachineBasicBlock::const_iterator(MI));
         RPT.advanceBeforeNext();
       }
 
       const GCNRPTracker::LiveRegSet LiveRegsCopy(RPT.getLiveRegs());
       RegUse Defs, Uses;
       if (!processRegUses(MI, Defs, Uses, RPT)) {
         RPT.reset(MI, &LiveRegsCopy);
         continue;
       }
 
       unsigned Length = 1;
       for ( ; Next != E && Length < FuncMaxClause; ++Next) {
+        // Debug instructions should not change the bundling. We need to move
+        // these after the bundle
+        if (Next->isDebugInstr())
+          continue;
+
         if (!isValidClauseInst(*Next, IsVMEM))
           break;
 
         // A load from pointer which was loaded inside the same bundle is an
         // impossible clause because we will need to write and read the same
         // register inside. In this case processRegUses will return false.
         if (!processRegUses(*Next, Defs, Uses, RPT))
           break;
 
         ++Length;
       }
       if (Length < 2) {
         RPT.reset(MI, &LiveRegsCopy);
         continue;
       }
 
       Changed = true;
       MFI->limitOccupancy(LastRecordedOccupancy);
 
       auto B = BuildMI(MBB, I, DebugLoc(), TII->get(TargetOpcode::BUNDLE));
       Ind->insertMachineInstrInMaps(*B);
 
       // Restore the state after processing the bundle.
       RPT.reset(*B, &LiveRegsCopy);
+      DbgInstrs.clear();
+
+      auto BundleNext = I;
+      for (auto BI = I; BI != Next; BI = BundleNext) {
+        BundleNext = std::next(BI);
+
+        if (BI->isDebugValue()) {
+          DbgInstrs.push_back(BI->removeFromParent());
+          continue;
+        }
 
-      for (auto BI = I; BI != Next; ++BI) {
         BI->bundleWithPred();
         Ind->removeSingleMachineInstrFromMaps(*BI);
 
         for (MachineOperand &MO : BI->defs())
           if (MO.readsReg())
             MO.setIsInternalRead(true);
       }
 
+      // Replace any debug instructions after the new bundle.
+      for (MachineInstr *DbgInst : DbgInstrs)
+        MBB.insert(Next, DbgInst);
+
       for (auto &&R : Defs) {
         forAllLanes(R.first, R.second.second, [&R, &B](unsigned SubReg) {
           unsigned S = R.second.first | RegState::EarlyClobber;
           if (!SubReg)
             S &= ~(RegState::Undef | RegState::Dead);
           B.addDef(R.first, S, SubReg);
         });
       }

llvm/test/CodeGen/AMDGPU/soft-clause-dbg-value.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx906 -mattr=+xnack -run-pass=si-form-memory-clauses -verify-machineinstrs -o - %s | FileCheck %s
+
+# Make sure that debug instructions do not change the bundling, and
+# the dbg_values which break the clause are inserted after the new
+# bundles.
+
+---
+name: sgpr_clause_dbg_value
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $sgpr4_sgpr5, $vgpr0, $vgpr1, $vgpr2, $vgpr3, $vgpr4, $vgpr5, $vgpr6, $vgpr7, $vgpr8, $vgpr9, $vgpr10, $vgpr11, $vgpr12, $vgpr13, $vgpr14, $vgpr15, $vgpr16, $vgpr17, $vgpr18, $vgpr19, $vgpr20, $vgpr21, $vgpr22, $vgpr23, $vgpr24
+    ; CHECK-LABEL: name: sgpr_clause_dbg_value
+    ; CHECK: liveins: $sgpr4_sgpr5, $vgpr0, $vgpr1, $vgpr2, $vgpr3, $vgpr4, $vgpr5, $vgpr6, $vgpr7, $vgpr8, $vgpr9, $vgpr10, $vgpr11, $vgpr12, $vgpr13, $vgpr14, $vgpr15, $vgpr16, $vgpr17, $vgpr18, $vgpr19, $vgpr20, $vgpr21, $vgpr22, $vgpr23, $vgpr24
+    ; CHECK: [[COPY:%[0-9]+]]:sreg_64 = COPY $sgpr4_sgpr5
+    ; CHECK: early-clobber %2:sreg_32_xm0_xexec, early-clobber %1:sreg_32_xm0_xexec = BUNDLE [[COPY]] {
+    ; CHECK:   [[S_LOAD_DWORD_IMM:%[0-9]+]]:sreg_32_xm0_xexec = S_LOAD_DWORD_IMM [[COPY]], 0, 0, 0 :: (load 4, addrspace 4)
+    ; CHECK:   [[S_LOAD_DWORD_IMM1:%[0-9]+]]:sreg_32_xm0_xexec = S_LOAD_DWORD_IMM [[COPY]], 8, 0, 0 :: (load 4, addrspace 4)
+    ; CHECK: }
+    ; CHECK: DBG_VALUE [[S_LOAD_DWORD_IMM]], 0, 0
+    ; CHECK: DBG_VALUE [[S_LOAD_DWORD_IMM1]], 0, 0
+    ; CHECK: S_NOP 0
+    ; CHECK: S_NOP 0
+    ; CHECK: S_NOP 0
+    ; CHECK: early-clobber %4:sreg_32_xm0_xexec, early-clobber %3:sreg_32_xm0_xexec, early-clobber %5:sreg_32_xm0_xexec = BUNDLE [[COPY]] {
+    ; CHECK:   [[S_LOAD_DWORD_IMM2:%[0-9]+]]:sreg_32_xm0_xexec = S_LOAD_DWORD_IMM [[COPY]], 16, 0, 0 :: (load 4, addrspace 4)
+    ; CHECK:   [[S_LOAD_DWORD_IMM3:%[0-9]+]]:sreg_32_xm0_xexec = S_LOAD_DWORD_IMM [[COPY]], 32, 0, 0 :: (load 4, addrspace 4)
+    ; CHECK:   [[S_LOAD_DWORD_IMM4:%[0-9]+]]:sreg_32_xm0_xexec = S_LOAD_DWORD_IMM [[COPY]], 64, 0, 0 :: (load 4, addrspace 4)
+    ; CHECK: }
+    ; CHECK: DBG_VALUE [[S_LOAD_DWORD_IMM2]], 0, 0
+    ; CHECK: DBG_VALUE [[S_LOAD_DWORD_IMM3]], 0, 0
+    ; CHECK: S_ENDPGM 0, implicit [[S_LOAD_DWORD_IMM]], implicit [[S_LOAD_DWORD_IMM1]], implicit [[S_LOAD_DWORD_IMM2]], implicit [[S_LOAD_DWORD_IMM3]], implicit [[S_LOAD_DWORD_IMM4]]
+    %0:sreg_64 = COPY $sgpr4_sgpr5
+    %1:sreg_32_xm0_xexec = S_LOAD_DWORD_IMM %0, 0, 0, 0 :: (load 4, align 4, addrspace 4)
+    DBG_VALUE %1, 0, 0
+    %2:sreg_32_xm0_xexec = S_LOAD_DWORD_IMM %0, 8, 0, 0 :: (load 4, align 4, addrspace 4)
+    DBG_VALUE %2, 0, 0
+    S_NOP 0
+    S_NOP 0
+    S_NOP 0
+    %3:sreg_32_xm0_xexec = S_LOAD_DWORD_IMM %0, 16, 0, 0 :: (load 4, align 4, addrspace 4)
+    %4:sreg_32_xm0_xexec = S_LOAD_DWORD_IMM %0, 32, 0, 0 :: (load 4, align 4, addrspace 4)
+    DBG_VALUE %3, 0, 0
+    DBG_VALUE %4, 0, 0
+    %5:sreg_32_xm0_xexec = S_LOAD_DWORD_IMM %0, 64, 0, 0 :: (load 4, align 4, addrspace 4)
+    S_ENDPGM 0, implicit %1, implicit %2, implicit %3, implicit %4, implicit %5
+
+...