MachineScheduler: Fix assert from not checking subregs

[arsenm]

Summary

The assert would fail if there was a dead def of a subregister if
there was a previous use of a different subregister.

[qcolombet]

Hi Matt,

Looks sensible.

The check is not quite right I think see inline comment (you should be able to write a test case to expose the issue), but the direction is the right one).

With that fixed LGTM.

Cheers,
-Quentin

lib/CodeGen/ScheduleDAGInstrs.cpp

383

We should check that the lane masks don't overlap instead.

test/CodeGen/AMDGPU/sched-assert-dead-def-subreg-use-other-subreg.mir

5

typo: indices

[arsenm]

r372287

include/llvm/CodeGen/ScheduleDAGInstrs.h

protected:
     ///
     /// Other adjustments may be made to the instruction if necessary. Return
     /// true if the operand has been deleted, false if not.
     void toggleKillFlag(MachineInstr &MI, MachineOperand &MO);
 
     /// Returns a mask for which lanes get read/written by the given (register)
     /// machine operand.
     LaneBitmask getLaneMaskForMO(const MachineOperand &MO) const;
+
+    /// Returns true if the def register in \p MO has no uses.
+    bool deadDefHasNoUse(const MachineOperand &MO);
   };
 
   /// Creates a new SUnit and return a ptr to it.
   inline SUnit *ScheduleDAGInstrs::newSUnit(MachineInstr *MI) {
 #ifndef NDEBUG
     const SUnit *Addr = SUnits.empty() ? nullptr : &SUnits[0];
 #endif
     SUnits.emplace_back(MI, (unsigned)SUnits.size());

lib/CodeGen/ScheduleDAGInstrs.cpp

if (!RC.HasDisjunctSubRegs)
     return LaneBitmask::getAll();
 
   unsigned SubReg = MO.getSubReg();
   if (SubReg == 0)
     return RC.getLaneMask();
   return TRI->getSubRegIndexLaneMask(SubReg);
 }
 
+bool ScheduleDAGInstrs::deadDefHasNoUse(const MachineOperand &MO) {
+  auto RegUse = CurrentVRegUses.find(MO.getReg());
+  if (RegUse == CurrentVRegUses.end())
+    return true;
+
+  // FIXME: SparseMultiSet prevents making method const
+  const MachineInstr *UseInst = RegUse->SU->getInstr();
+  return UseInst->getOperand(RegUse->OperandIndex).getSubReg() != MO.getSubReg();
+}
+
 /// Adds register output and data dependencies from this SUnit to instructions
 /// that occur later in the same scheduling region if they read from or write to
 /// the virtual register defined at OperIdx.
 ///
 /// TODO: Hoist loop induction variable increments. This has to be
 /// reevaluated. Generally, IV scheduling should be done before coalescing.
 void ScheduleDAGInstrs::addVRegDefDeps(SUnit *SU, unsigned OperIdx) {
   MachineInstr *MI = SU->getInstr();
if (TrackLaneMasks) {
     // Def is first.
     MO.setIsUndef(false);
   } else {
     DefLaneMask = LaneBitmask::getAll();
     KillLaneMask = LaneBitmask::getAll();
   }
 
   if (MO.isDead()) {
-    assert(CurrentVRegUses.find(Reg) == CurrentVRegUses.end() &&
-           "Dead defs should have no uses");
+    assert(deadDefHasNoUse(MO) && "Dead defs should have no uses");
   } else {
     // Add data dependence to all uses we found so far.
     const TargetSubtargetInfo &ST = MF.getSubtarget();
     for (VReg2SUnitOperIdxMultiMap::iterator I = CurrentVRegUses.find(Reg),
          E = CurrentVRegUses.end(); I != E; /*empty*/) {
       LaneBitmask LaneMask = I->LaneMask;
       // Ignore uses of other lanes.
       if ((LaneMask & KillLaneMask).none()) {

test/CodeGen/AMDGPU/sched-assert-dead-def-subreg-use-other-subreg.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx906 -verify-machineinstrs  -run-pass=machine-scheduler -verify-misched -o - %s | FileCheck %s
+
+# This would assert that a dead def should have no uses, but the dead
+# def and use have different subreg indicise.
+
+---
+name:            multi_def_dead_reg_subreg_check
+tracksRegLiveness: true
+machineFunctionInfo:
+  isEntryFunction: true
+  scratchRSrcReg:  '$sgpr24_sgpr25_sgpr26_sgpr27'
+  scratchWaveOffsetReg: '$sgpr32'
+  frameOffsetReg:  '$sgpr32'
+  stackPtrOffsetReg: '$sgpr32'
+  argumentInfo:
+    privateSegmentBuffer: { reg: '$sgpr0_sgpr1_sgpr2_sgpr3' }
+    privateSegmentWaveByteOffset: { reg: '$sgpr33' }
+body:             |
+  ; CHECK-LABEL: name: multi_def_dead_reg_subreg_check
+  ; CHECK: bb.0:
+  ; CHECK:   successors: %bb.1(0x80000000)
+  ; CHECK:   liveins: $sgpr6_sgpr7
+  ; CHECK:   undef %0.sub3:vreg_512 = V_MOV_B32_e32 0, implicit $exec
+  ; CHECK:   [[V_MOV_B32_e32_:%[0-9]+]]:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+  ; CHECK:   [[V_ADD_U32_e32_:%[0-9]+]]:vgpr_32 = V_ADD_U32_e32 0, [[V_MOV_B32_e32_]], implicit $exec
+  ; CHECK:   [[V_MOV_B32_e32_1:%[0-9]+]]:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+  ; CHECK:   [[COPY:%[0-9]+]]:vreg_512 = COPY %0
+  ; CHECK: bb.1:
+  ; CHECK:   successors: %bb.1(0x80000000)
+  ; CHECK:   BUFFER_STORE_DWORD_OFFEN %0.sub3, undef %5:vgpr_32, $sgpr24_sgpr25_sgpr26_sgpr27, $sgpr32, 0, 0, 0, 0, 0, implicit $exec :: (store 4, align 8, addrspace 5)
+  ; CHECK:   dead %6:vgpr_32 = DS_READ_B32_gfx9 undef %7:vgpr_32, 0, 0, implicit $exec
+  ; CHECK:   dead %8:vreg_64 = DS_READ_B64_gfx9 [[V_MOV_B32_e32_]], 0, 0, implicit $exec
+  ; CHECK:   dead %9:vreg_128 = DS_READ_B128_gfx9 [[V_ADD_U32_e32_]], 0, 0, implicit $exec
+  ; CHECK:   [[COPY1:%[0-9]+]]:vgpr_32 = COPY [[COPY]].sub0
+  ; CHECK:   undef %11.sub1:vreg_512 = COPY [[COPY]].sub1
+  ; CHECK:   INLINEASM &"", 1, 851978, def dead [[COPY1]], 851978, def dead [[COPY]].sub1, 2147483657, [[COPY1]], 2147549193, [[COPY]].sub1
+  ; CHECK:   %11.sub0:vreg_512 = COPY [[COPY]].sub0
+  ; CHECK:   %11.sub3:vreg_512 = COPY [[COPY]].sub3
+  ; CHECK:   dead %10:vgpr_32 = V_ADD_I32_e32 4, [[V_MOV_B32_e32_1]], implicit-def dead $vcc, implicit $exec
+  ; CHECK:   %11.sub2:vreg_512 = COPY undef [[V_MOV_B32_e32_]]
+  ; CHECK:   %11.sub5:vreg_512 = COPY undef [[V_MOV_B32_e32_]]
+  ; CHECK:   [[COPY2:%[0-9]+]]:vreg_512 = COPY %11
+  ; CHECK:   S_BRANCH %bb.1
+  bb.0:
+    liveins: $sgpr6_sgpr7
+
+    undef %0.sub3:vreg_512 = V_MOV_B32_e32 0, implicit $exec
+    %1:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+    %2:vgpr_32 = V_ADD_U32_e32 0, %1, implicit $exec
+    %3:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+    %4:vreg_512 = COPY %0
+
+  bb.1:
+    BUFFER_STORE_DWORD_OFFEN %0.sub3, undef %5:vgpr_32, $sgpr24_sgpr25_sgpr26_sgpr27, $sgpr32, 0, 0, 0, 0, 0, implicit $exec :: (store 4, align 8, addrspace 5)
+    %6:vgpr_32 = DS_READ_B32_gfx9 undef %7:vgpr_32, 0, 0, implicit $exec
+    %8:vreg_64 = DS_READ_B64_gfx9 %1, 0, 0, implicit $exec
+    %9:vreg_128 = DS_READ_B128_gfx9 %2, 0, 0, implicit $exec
+    %10:vgpr_32 = V_ADD_I32_e32 4, %3, implicit-def dead $vcc, implicit $exec
+    undef %11.sub0:vreg_512 = COPY %4.sub0
+    %12:vgpr_32 = COPY %4.sub0
+    %11.sub1:vreg_512 = COPY %4.sub1
+    INLINEASM &"", 1, 851978, def dead %12, 851978, def dead %4.sub1, 2147483657, %12, 2147549193, %4.sub1
+    %11.sub2:vreg_512 = COPY undef %1
+    %11.sub3:vreg_512 = COPY %4.sub3
+    %11.sub5:vreg_512 = COPY undef %1
+    %4:vreg_512 = COPY %11
+    S_BRANCH %bb.1
+
+...