[AMDGPU] Check MI bundles for hazards

[kerbowa]

Summary

GCNHazardRecognizer fails to identify hazards that are in and around bundles. This patch allows the hazard recognizer to consider bundled instructions in both scheduler and hazard recognizer mode. We ignore “bundledness” for the purpose of detecting hazards and examine the instructions individually.

[rampitec]

Can you add a test for one of the hazards which do not expire and have no max waitstaits? Such as hazards from smem-war-hazard.mir for example.
Granted we do not do it today, but you can write a bundle with such instructions manually.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

1172

I do not like code duplication here. Can you call one from another?

[kerbowa]

Add 'fixHazard' checks.

[kerbowa]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

1172

At first glance having the standard insertWaitStates call the instr_iterator version seems to break a bunch of tests. I can just add a function to insert noop's in bundles in GCNHazardRecognizer instead.

[arsenm]

Can you add a separate test file for cases where a hazard was hidden by a bundle, and another for where the hazard is inside the bundle?

[rampitec]

In D61564#1491648, @arsenm wrote:

Can you add a separate test file for cases where a hazard was hidden by a bundle, and another for where the hazard is inside the bundle?

LGTM, but I am second to that.

[kerbowa]

Add seperate test files.

[rampitec]

There is no reason to split two test files.
For the test itself, could you please add one more. You have test where hazard instruction follows bundle. Can you add the same, where it is in bundle, and another one precedes it? Thanks!

[arsenm]

The separate files helps me read which test is for which purpose

[rampitec]

In D61564#1493374, @arsenm wrote:

The separate files helps me read which test is for which purpose

Ok, let them be separate.

[kerbowa]

In D61564#1492989, @rampitec wrote:

For the test itself, could you please add one more. You have test where hazard instruction follows bundle. Can you add the same, where it is in bundle, and another one precedes it? Thanks!

Sorry, I'm not sure I understand exactly what you mean. Do you want a hazard instruction in a bundle with another hazard instruction preceding it?

[rampitec]

In D61564#1494113, @kerbowa wrote:

In D61564#1492989, @rampitec wrote:

For the test itself, could you please add one more. You have test where hazard instruction follows bundle. Can you add the same, where it is in bundle, and another one precedes it? Thanks!

Sorry, I'm not sure I understand exactly what you mean. Do you want a hazard instruction in a bundle with another hazard instruction preceding it?

Right.

[kerbowa]

Add new test.

[rampitec]

LGTM

llvm/lib/Target/AMDGPU/GCNHazardRecognizer.h

private:
 
   void resetClause() {
     ClauseUses.reset();
     ClauseDefs.reset();
   }
 
   void addClauseInst(const MachineInstr &MI);
 
+  // Advance over a MachineInstr bundle. Look for hazards in the bundled
+  // instructions.
+  void processBundle();
+
   int getWaitStatesSince(IsHazardFn IsHazard, int Limit);
   int getWaitStatesSinceDef(unsigned Reg, IsHazardFn IsHazardDef, int Limit);
   int getWaitStatesSinceSetReg(IsHazardFn IsHazard, int Limit);
 
   int checkSoftClauseHazards(MachineInstr *SMEM);
   int checkSMRDHazards(MachineInstr *SMRD);
   int checkVMEMHazards(MachineInstr* VMEM);
   int checkDPPHazards(MachineInstr *DPP);

llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp

static unsigned getHWReg(const SIInstrInfo *TII, const MachineInstr &RegInstr) {
   const MachineOperand *RegOp = TII->getNamedOperand(RegInstr,
                                                      AMDGPU::OpName::simm16);
   return RegOp->getImm() & AMDGPU::Hwreg::ID_MASK_;
 }
 
 ScheduleHazardRecognizer::HazardType
 GCNHazardRecognizer::getHazardType(SUnit *SU, int Stalls) {
   MachineInstr *MI = SU->getInstr();
+  if (MI->isBundle())
+   return NoHazard;
 
   if (SIInstrInfo::isSMRD(*MI) && checkSMRDHazards(MI) > 0)
     return NoopHazard;
 
   // FIXME: Should flat be considered vmem?
   if ((SIInstrInfo::isVMEM(*MI) ||
        SIInstrInfo::isFLAT(*MI))
       && checkVMEMHazards(MI) > 0)
if (MI->isInlineAsm() && checkInlineAsmHazards(MI) > 0)
     return NoopHazard;
 
   if (checkAnyInstHazards(MI) > 0)
     return NoopHazard;
 
   return NoHazard;
 }
 
+void GCNHazardRecognizer::processBundle() {
+  MachineBasicBlock::instr_iterator MI = std::next(CurrCycleInstr->getIterator());
+  MachineBasicBlock::instr_iterator E = CurrCycleInstr->getParent()->instr_end();
+  // Check bundled MachineInstr's for hazards.
+  for (; MI != E && MI->isInsideBundle(); ++MI) {
+    CurrCycleInstr = &*MI;
+    unsigned WaitStates = PreEmitNoopsCommon(CurrCycleInstr);
+
+    for (unsigned i = 0; i < WaitStates; ++i)
+      TII.insertNoop(*MI->getParent(), MI);
+
+    // It’s unnecessary to track more than MaxLookAhead instructions. Since we
+    // include the bundled MI directly after, only add a maximum of
+    // (MaxLookAhead - 1) noops to EmittedInstrs.
+    for (unsigned i = 0, e = std::min(WaitStates, MaxLookAhead - 1); i < e; ++i)
+      EmittedInstrs.push_front(nullptr);
+
+    EmittedInstrs.push_front(CurrCycleInstr);
+    EmittedInstrs.resize(MaxLookAhead);
+  }
+  CurrCycleInstr = nullptr;
+}
+
 unsigned GCNHazardRecognizer::PreEmitNoops(SUnit *SU) {
   IsHazardRecognizerMode = false;
   return PreEmitNoopsCommon(SU->getInstr());
 }
 
 unsigned GCNHazardRecognizer::PreEmitNoops(MachineInstr *MI) {
   IsHazardRecognizerMode = true;
   CurrCycleInstr = MI;
   unsigned W = PreEmitNoopsCommon(MI);
 
   fixVMEMtoScalarWriteHazards(MI);
   fixSMEMtoVectorWriteHazards(MI);
   fixVcmpxExecWARHazard(MI);
   fixLdsBranchVmemWARHazard(MI);
 
   CurrCycleInstr = nullptr;
   return W;
 }
 
 unsigned GCNHazardRecognizer::PreEmitNoopsCommon(MachineInstr *MI) {
+  if (MI->isBundle())
+    return 0;
+
   int WaitStates = std::max(0, checkAnyInstHazards(MI));
 
   if (SIInstrInfo::isSMRD(*MI))
     return std::max(WaitStates, checkSMRDHazards(MI));
 
   if (SIInstrInfo::isVMEM(*MI) || SIInstrInfo::isFLAT(*MI))
     WaitStates = std::max(WaitStates, checkVMEMHazards(MI));
 
void GCNHazardRecognizer::AdvanceCycle() {
 
   // Do not track non-instructions which do not affect the wait states.
   // If included, these instructions can lead to buffer overflow such that
   // detectable hazards are missed.
   if (CurrCycleInstr->isImplicitDef() || CurrCycleInstr->isDebugInstr() ||
       CurrCycleInstr->isKill())
     return;
 
+  if (CurrCycleInstr->isBundle()) {
+    processBundle();
+    return;
+  }
+
+
   unsigned NumWaitStates = TII.getNumWaitStates(*CurrCycleInstr);
 
   // Keep track of emitted instructions
   EmittedInstrs.push_front(CurrCycleInstr);
 
   // Add a nullptr for each additional wait state after the first.  Make sure
   // not to add more than getMaxLookAhead() items to the list, since we
   // truncate the list to that size right after this loop.

 // Only scans until \p IsExpired does not return true.
 // Can only be run in a hazard recognizer mode.
 static int getWaitStatesSince(GCNHazardRecognizer::IsHazardFn IsHazard,
                               MachineBasicBlock *MBB,
                               MachineBasicBlock::reverse_instr_iterator I,
                               int WaitStates,
                               IsExpiredFn IsExpired,
                               DenseSet<const MachineBasicBlock *> &Visited) {
+  for (auto E = MBB->instr_rend(); I != E; ++I) {
+    // Don't add WaitStates for parent BUNDLE instructions.
+    if (I->isBundle())
+      continue;
 
-  for (auto E = MBB->rend() ; I != E; ++I) {
     if (IsHazard(&*I))
       return WaitStates;
 
     if (I->isInlineAsm() || I->isImplicitDef() || I->isDebugInstr())
       continue;
 
     WaitStates += SIInstrInfo::getNumWaitStates(*I);
 
int GCNHazardRecognizer::checkSoftClauseHazards(MachineInstr *MEM) {
   bool IsSMRD = TII.isSMRD(*MEM);
 
   resetClause();
 
   // A soft-clause is any group of consecutive SMEM instructions.  The
   // instructions in this group may return out of order and/or may be
   // replayed (i.e. the same instruction issued more than once).
   //
-  // In order to handle these situations correctly we need to make sure
-  // that when a clause has more than one instruction, no instruction in the
-  // clause writes to a register that is read another instruction in the clause
+  // In order to handle these situations correctly we need to make sure that
+  // when a clause has more than one instruction, no instruction in the clause
+  // writes to a register that is read by another instruction in the clause
   // (including itself). If we encounter this situaion, we need to break the
   // clause by inserting a non SMEM instruction.
 
   for (MachineInstr *MI : EmittedInstrs) {
     // When we hit a non-SMEM instruction then we have passed the start of the
     // clause and we can stop.
     if (!MI)
       break;
if (ST.getGeneration() < AMDGPUSubtarget::VOLCANIC_ISLANDS)
     return 0;
 
   int WaitStatesNeeded = checkSoftClauseHazards(VMEM);
 
   // A read of an SGPR by a VMEM instruction requires 5 wait states when the
   // SGPR was written by a VALU Instruction.
   const int VmemSgprWaitStates = 5;
   auto IsHazardDefFn = [this] (MachineInstr *MI) { return TII.isVALU(*MI); };
-
   for (const MachineOperand &Use : VMEM->uses()) {
     if (!Use.isReg() || TRI.isVGPR(MF.getRegInfo(), Use.getReg()))
       continue;
 
     int WaitStatesNeededForUse =
         VmemSgprWaitStates - getWaitStatesSinceDef(Use.getReg(), IsHazardDefFn,
                                                    VmemSgprWaitStates);
     WaitStatesNeeded = std::max(WaitStatesNeeded, WaitStatesNeededForUse);

llvm/lib/Target/AMDGPU/SIInstrInfo.h

public:
   /// Replace this instruction's opcode with the equivalent VALU
   /// opcode.  This function will also move the users of \p MI to the
   /// VALU if necessary. If present, \p MDT is updated.
   void moveToVALU(MachineInstr &MI, MachineDominatorTree *MDT = nullptr) const;
 
   void insertWaitStates(MachineBasicBlock &MBB,MachineBasicBlock::iterator MI,
                         int Count) const;
 
+  void insertWaitStates(MachineBasicBlock &MBB,MachineBasicBlock::instr_iterator MI,
+                        int Count) const;
+
   void insertNoop(MachineBasicBlock &MBB,
                   MachineBasicBlock::iterator MI) const override;
 
+  // Insert a noop using an instr_iterator. If MI is in a bundle the inserted
+  // noop will be included in the same bundle.
+  void insertNoop(MachineBasicBlock &MBB,
+                  MachineBasicBlock::instr_iterator MI) const;
+
   void insertReturn(MachineBasicBlock &MBB) const;
   /// Return the number of wait states that result from executing this
   /// instruction.
   static unsigned getNumWaitStates(const MachineInstr &MI);
 
   /// Returns the operand named \p Op.  If \p MI does not have an
   /// operand named \c Op, this function returns nullptr.
   LLVM_READONLY

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

while (Count > 0) {
     else
       Arg = Count - 1;
     Count -= 8;
     BuildMI(MBB, MI, DL, get(AMDGPU::S_NOP))
             .addImm(Arg);
   }
 }
 
+void SIInstrInfo::insertWaitStates(MachineBasicBlock &MBB,
+                                   MachineBasicBlock::instr_iterator MI,
+                                   int Count) const {
+  DebugLoc DL = MBB.findDebugLoc(MI);
+  while (Count > 0) {
+    int Arg;
+    if (Count >= 8)
+      Arg = 7;
+    else
+      Arg = Count - 1;
+    Count -= 8;
+    BuildMI(MBB, MI, DL, get(AMDGPU::S_NOP))
+            .addImm(Arg);
+  }
+}
+
 void SIInstrInfo::insertNoop(MachineBasicBlock &MBB,
                              MachineBasicBlock::iterator MI) const {
   insertWaitStates(MBB, MI, 1);
 }
 
+void SIInstrInfo::insertNoop(MachineBasicBlock &MBB,
+                             MachineBasicBlock::instr_iterator MI) const {
+  insertWaitStates(MBB, MI, 1);
+}
+
 void SIInstrInfo::insertReturn(MachineBasicBlock &MBB) const {
   auto MF = MBB.getParent();
   SIMachineFunctionInfo *Info = MF->getInfo<SIMachineFunctionInfo>();
 
   assert(Info->isEntryFunction());
 
   if (MBB.succ_empty()) {
     bool HasNoTerminator = MBB.getFirstTerminator() == MBB.end();

llvm/test/CodeGen/AMDGPU/hazard-bundle.mir

+# RUN: llc -march=amdgcn -mcpu=gfx902 -verify-machineinstrs -run-pass  post-RA-hazard-rec %s -o - | FileCheck -check-prefixes=GCN,XNACK %s
+# RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -run-pass  post-RA-hazard-rec %s -o - | FileCheck -check-prefixes=GCN,NOXNACK %s
+
+# GCN-LABEL:    name: break_smem_clause_simple_load_smrd8_ptr_before_bundle
+# GCN:          BUNDLE
+# XNACK-NEXT:   S_NOP
+# NOXNACK-NOT:  S_NOP
+# GCN:          }
+# XNACK-NEXT:   S_NOP
+# NOXNACK-NOT:  S_NOP
+# GCN:          S_LOAD_DWORDX2_IMM
+---
+name: break_smem_clause_simple_load_smrd8_ptr_before_bundle
+
+body: |
+  bb.0:
+    $sgpr0_sgpr1 = S_LOAD_DWORDX2_IMM $sgpr10_sgpr11, 0, 0, 0
+    BUNDLE implicit-def $sgpr6_sgpr7 {
+      $sgpr10_sgpr11 = S_LOAD_DWORDX2_IMM $sgpr12_sgpr13, 0, 0, 0
+    }
+    $sgpr12_sgpr13 = S_LOAD_DWORDX2_IMM $sgpr14_sgpr15, 0, 0, 0
+    S_ENDPGM 0
+...
+
+# GCN-LABEL:    name: vmem_vcc_hazard_ignore_bundle_instr
+# GCN:          BUNDLE
+# GCN:          BUNDLE
+# GCN-NEXT:     S_NOP
+# GCN-NEXT:     S_NOP
+# GCN-NEXT:     S_NOP
+# GCN-NEXT:     S_NOP
+# GCN-NEXT:     S_NOP
+# GCN:          BUFFER_LOAD_DWORD_OFFEN
+---
+name: vmem_vcc_hazard_ignore_bundle_instr
+body: |
+  bb.0:
+    $sgpr0_sgpr1_sgpr2_sgpr3 = IMPLICIT_DEF
+    $vgpr0 = IMPLICIT_DEF
+    BUNDLE implicit-def $vgpr1, implicit $vgpr0, implicit $vgpr0, implicit-def $vcc, implicit $vcc, implicit $exec {
+      $vgpr1 = V_ADDC_U32_e32 $vgpr0, $vgpr0, implicit-def $vcc, implicit $vcc, implicit $exec
+    }
+    BUNDLE implicit-def $vgpr1, implicit $vgpr0, implicit $sgpr0_sgpr1_sgpr2_sgpr3, implicit $vcc_lo, implicit $exec {
+      $vgpr1 = BUFFER_LOAD_DWORD_OFFEN $vgpr0, $sgpr0_sgpr1_sgpr2_sgpr3, $vcc_lo, 0, 0, 0, 0, 0, implicit $exec
+    }
+    S_ENDPGM 0
+...
+
+# GCN-LABEL: name: vmem_vcc_min_of_two_in_bundle
+# GCN:      BUNDLE
+# GCN-NEXT: S_NOP
+# GCN-NEXT: S_NOP
+# GCN-NEXT: S_NOP
+# GCN-NEXT: S_NOP
+# GCN-NEXT: S_NOP
+# GCN-NEXT: BUFFER_LOAD_DWORD_OFFEN
+---
+name: vmem_vcc_min_of_two_in_bundle
+body: |
+  bb.0:
+    successors: %bb.2
+
+    $sgpr0_sgpr1_sgpr2_sgpr3 = IMPLICIT_DEF
+    $vgpr0 = IMPLICIT_DEF
+    $vgpr1 = V_ADDC_U32_e32 $vgpr0, $vgpr0, implicit-def $vcc, implicit $vcc, implicit $exec
+    S_NOP 0
+    S_BRANCH %bb.2
+
+  bb.1:
+    successors: %bb.2
+
+    $vgpr1 = V_ADDC_U32_e32 $vgpr0, $vgpr0, implicit-def $vcc, implicit $vcc, implicit $exec
+
+  bb.2:
+    BUNDLE implicit-def $vgpr1, implicit $vgpr0, implicit $sgpr0_sgpr1_sgpr2_sgpr3, implicit $vcc_lo, implicit $exec {
+      $vgpr1 = BUFFER_LOAD_DWORD_OFFEN $vgpr0, $sgpr0_sgpr1_sgpr2_sgpr3, $vcc_lo, 0, 0, 0, 0, 0, implicit $exec
+    }
+...