[AMDGPU] Improve hazard checks for inline asm statements

[msearles]

Summary

[AMDGPU] Add GCNHazardRecognizer::checkInlineAsmHazards() and GCNHazardRecognizer::checkVALUHazardsHelper(). checkInlineAsmHazards() checks INLINEASM for hazards that we particularly care about (so not exhaustive); this patch adds a check for INLINEASM that defs vregs that hold data-to-be stored by immediately preceding store of more than 8 bytes. If the instr were not within an INLINEASM, this scenario would be handled by checkVALUHazard(). Add checkVALUHazardsHelper(), which will be called by both checkVALUHazards() and checkInlineAsmHazards().

[arsenm]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

577 ↗

(On Diff #123075)

Should this check every instruction type hazard?

[msearles]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

577 ↗

(On Diff #123075)

Probably. I'm fine to add a FIXME to checkInlineAsmHazards() to not forget that more work needs to be done and fine to do that work. However, I'd like to push the fix for this bug, since it is being hit and add the additional code in a follow-on patch.

[msearles]

Ping

[arsenm]

LGTM with formatting fixed

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

519 ↗

(On Diff #123075)

This looks like it goes over 80 lines

548 ↗

(On Diff #123075)

MF.getRegInfo()

[msearles]

Adjusted per reviewer comments.

[msearles]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

519 ↗

(On Diff #123075)

Fixed

548 ↗

(On Diff #123075)

Fixed this instance as well as a few others.

llvm/trunk/lib/Target/AMDGPU/GCNHazardRecognizer.h

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/CodeGen/ScheduleHazardRecognizer.h"
 #include <list>
 
 namespace llvm {
 
 class MachineFunction;
 class MachineInstr;
+class MachineOperand;
+class MachineRegisterInfo;
 class ScheduleDAG;
 class SIInstrInfo;
 class SIRegisterInfo;
 class SISubtarget;
 
 class GCNHazardRecognizer final : public ScheduleHazardRecognizer {
   // This variable stores the instruction that has been emitted this cycle. It
   // will be added to EmittedInstrs, when AdvanceCycle() or RecedeCycle() is
class GCNHazardRecognizer final : public ScheduleHazardRecognizer {
   int checkSMRDHazards(MachineInstr *SMRD);
   int checkVMEMHazards(MachineInstr* VMEM);
   int checkDPPHazards(MachineInstr *DPP);
   int checkDivFMasHazards(MachineInstr *DivFMas);
   int checkGetRegHazards(MachineInstr *GetRegInstr);
   int checkSetRegHazards(MachineInstr *SetRegInstr);
   int createsVALUHazard(const MachineInstr &MI);
   int checkVALUHazards(MachineInstr *VALU);
+  int checkVALUHazardsHelper(const MachineOperand &Def, const MachineRegisterInfo &MRI);
   int checkRWLaneHazards(MachineInstr *RWLane);
   int checkRFEHazards(MachineInstr *RFE);
+  int checkInlineAsmHazards(MachineInstr *IA);
   int checkAnyInstHazards(MachineInstr *MI);
   int checkReadM0Hazards(MachineInstr *SMovRel);
 public:
   GCNHazardRecognizer(const MachineFunction &MF);
   // We can only issue one instruction per cycle.
   bool atIssueLimit() const override { return true; }
   void EmitInstruction(SUnit *SU) override;
   void EmitInstruction(MachineInstr *MI) override;

llvm/trunk/lib/Target/AMDGPU/GCNHazardRecognizer.cpp

if (ST.hasReadM0MovRelInterpHazard() &&
       (TII.isVINTRP(*MI) || isSMovRel(MI->getOpcode())) &&
       checkReadM0Hazards(MI) > 0)
     return NoopHazard;
 
   if (ST.hasReadM0SendMsgHazard() && isSendMsgTraceDataOrGDS(*MI) &&
       checkReadM0Hazards(MI) > 0)
     return NoopHazard;
 
+  if (MI->isInlineAsm() && checkInlineAsmHazards(MI) > 0)
+    return NoopHazard;
+
   if (checkAnyInstHazards(MI) > 0)
     return NoopHazard;
 
   return NoHazard;
 }
 
 unsigned GCNHazardRecognizer::PreEmitNoops(SUnit *SU) {
   return PreEmitNoops(SU->getInstr());
if (SIInstrInfo::isDPP(*MI))
     WaitStates = std::max(WaitStates, checkDPPHazards(MI));
 
   if (isDivFMas(MI->getOpcode()))
     WaitStates = std::max(WaitStates, checkDivFMasHazards(MI));
 
   if (isRWLane(MI->getOpcode()))
     WaitStates = std::max(WaitStates, checkRWLaneHazards(MI));
 
+  if (MI->isInlineAsm())
+    return std::max(WaitStates, checkInlineAsmHazards(MI));
+
   if (isSGetReg(MI->getOpcode()))
     return std::max(WaitStates, checkGetRegHazards(MI));
 
   if (isSSetReg(MI->getOpcode()))
     return std::max(WaitStates, checkSetRegHazards(MI));
 
   if (isRFE(MI->getOpcode()))
     return std::max(WaitStates, checkRFEHazards(MI));
if (TII->isFLAT(MI)) {
     int DataIdx = AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::vdata);
     if (AMDGPU::getRegBitWidth(Desc.OpInfo[DataIdx].RegClass) > 64)
       return DataIdx;
   }
 
   return -1;
 }
 
-int GCNHazardRecognizer::checkVALUHazards(MachineInstr *VALU) {
-  // This checks for the hazard where VMEM instructions that store more than
+int GCNHazardRecognizer::checkVALUHazardsHelper(const MachineOperand &Def,
+            const MachineRegisterInfo &MRI) {
+  // Helper to check for the hazard where VMEM instructions that store more than
   // 8 bytes can have there store data over written by the next instruction.
-  if (!ST.has12DWordStoreHazard())
-    return 0;
-
   const SIRegisterInfo *TRI = ST.getRegisterInfo();
-  const MachineRegisterInfo &MRI = VALU->getParent()->getParent()->getRegInfo();
 
   const int VALUWaitStates = 1;
   int WaitStatesNeeded = 0;
 
-  for (const MachineOperand &Def : VALU->defs()) {
-    if (!TRI->isVGPR(MRI, Def.getReg()))
-      continue;
-    unsigned Reg = Def.getReg();
-    auto IsHazardFn = [this, Reg, TRI] (MachineInstr *MI) {
-      int DataIdx = createsVALUHazard(*MI);
-      return DataIdx >= 0 &&
-             TRI->regsOverlap(MI->getOperand(DataIdx).getReg(), Reg);
-    };
-    int WaitStatesNeededForDef =
-        VALUWaitStates - getWaitStatesSince(IsHazardFn);
-    WaitStatesNeeded = std::max(WaitStatesNeeded, WaitStatesNeededForDef);
-  }
+    return WaitStatesNeeded;
+
+  return WaitStatesNeeded;
+
+int GCNHazardRecognizer::checkVALUHazards(MachineInstr *VALU) {
+  // This checks for the hazard where VMEM instructions that store more than
+  // 8 bytes can have there store data over written by the next instruction.
+  if (!ST.has12DWordStoreHazard())
+    return 0;
+
+  const MachineRegisterInfo &MRI = MF.getRegInfo();
+  int WaitStatesNeeded = 0;
+
+  for (const MachineOperand &Def : VALU->defs()) {
+    WaitStatesNeeded = std::max(WaitStatesNeeded, checkVALUHazardsHelper(Def, MRI));
+  }
+
+  return WaitStatesNeeded;
+}
+
+int GCNHazardRecognizer::checkInlineAsmHazards(MachineInstr *IA) {
+  // This checks for hazards associated with inline asm statements.
+  // Since inline asms can contain just about anything, we use this
+  // to call/leverage other check*Hazard routines. Note that
+  // this function doesn't attempt to address all possible inline asm
+  // hazards (good luck), but is a collection of what has been
+  // problematic thus far.
+
+  // see checkVALUHazards()
+  if (!ST.has12DWordStoreHazard())
+    return 0;
+
+  const MachineRegisterInfo &MRI = MF.getRegInfo();
+  int WaitStatesNeeded = 0;
+
+  for (unsigned I = InlineAsm::MIOp_FirstOperand, E = IA->getNumOperands();
+       I != E; ++I) {
+    const MachineOperand &Op = IA->getOperand(I);
+    if (Op.isReg() && Op.isDef()) {
+      WaitStatesNeeded = std::max(WaitStatesNeeded, checkVALUHazardsHelper(Op, MRI));
+    }
+  }
+
   return WaitStatesNeeded;
 }
 
 int GCNHazardRecognizer::checkRWLaneHazards(MachineInstr *RWLane) {
   const SIInstrInfo *TII = ST.getInstrInfo();
   const SIRegisterInfo *TRI = ST.getRegisterInfo();
-  const MachineRegisterInfo &MRI =
-      RWLane->getParent()->getParent()->getRegInfo();
+  const MachineRegisterInfo &MRI = MF.getRegInfo();
 
   const MachineOperand *LaneSelectOp =
       TII->getNamedOperand(*RWLane, AMDGPU::OpName::src1);
 
   if (!LaneSelectOp->isReg() || !TRI->isSGPRReg(MRI, LaneSelectOp->getReg()))
     return 0;
 
   unsigned LaneSelectReg = LaneSelectOp->getReg();

llvm/trunk/test/CodeGen/AMDGPU/hazard-inlineasm.mir

+# RUN: llc -mcpu=gfx900 -march=amdgcn -verify-machineinstrs -run-pass post-RA-hazard-rec %s -o - | FileCheck %s
+
+# If an INLINEASM statement is preceded by a vmem store of more than 8 bytes *and*
+# the INLINEASM defs the vregs holding the data-to-be-stored by that preceding store,
+# then the hazard recognizer should insert a s_nop in between them.
+
+...
+
+# GCN-LABEL: name: hazard-inlineasm
+# CHECK: FLAT_STORE_DWORDX4
+# CHECK-NEXT: S_NOP 0
+# CHECK-NEXT: INLINEASM
+
+---
+name: hazard-inlineasm
+
+body: |
+  bb.0:
+   FLAT_STORE_DWORDX4 %vgpr49_vgpr50, %vgpr26_vgpr27_vgpr28_vgpr29, 0, 0, 0, implicit %exec, implicit %flat_scr
+   INLINEASM $"v_mad_u64_u32 $0, $1, $2, $3, $4", 0, 2621450, def %vgpr26_vgpr27, 2818058, def dead %sgpr14_sgpr15, 589833, %sgpr12, 327689, killed %vgpr51, 2621449, %vgpr46_vgpr47
+   S_ENDPGM
+...
+
+