[AMDGPU] Fix bug in S_MOVREL check (PR32248)

[arsenm]

Summary

https://bugs.llvm.org/show_bug.cgi?id=32248

Fix the isSMovRel check to correctly test for an opcode instead of using the opcode directly as a condition - resulting in an always true result.

[tstellar]

test/CodeGen/AMDGPU/inserted-wait-states.mir

456–471 ↗

(On Diff #91996)

Aren't these nops are still required. Have you checked the GFX9 ISA guide?

[arsenm]

test/CodeGen/AMDGPU/inserted-wait-states.mir

456–471 ↗

(On Diff #91996)

I don't think it's been published yet. I think these are required though

[RKSimon]

test/CodeGen/AMDGPU/inserted-wait-states.mir

456–471 ↗

(On Diff #91996)

In which case the correct codegen is relying on the always true result from isSMovRel.....

[arsenm]

test/CodeGen/AMDGPU/inserted-wait-states.mir

456–471 ↗

(On Diff #91996)

I think I found the problem, there is another place that is supposed to check the same condition. I'll commit this soon

[arsenm]

Fix not inserting nops for VINTRP read m0 after SALU write m0 hazard

[RKSimon]

Looks ok to me, but I just created the original patch to fix the pvs warning.

[arsenm]

r298125

lib/Target/AMDGPU/GCNHazardRecognizer.h

 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
   // called.
   MachineInstr *CurrCycleInstr;
   std::list<MachineInstr*> EmittedInstrs;
   const MachineFunction &MF;
   const SISubtarget &ST;
+  const SIInstrInfo &TII;
 
   int getWaitStatesSince(function_ref<bool(MachineInstr *)> IsHazard);
   int getWaitStatesSinceDef(unsigned Reg,
                             function_ref<bool(MachineInstr *)> IsHazardDef =
                                 [](MachineInstr *) { return true; });
   int getWaitStatesSinceSetReg(function_ref<bool(MachineInstr *)> IsHazard);
 
   int checkSMEMSoftClauseHazards(MachineInstr *SMEM);

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

 
 //===----------------------------------------------------------------------===//
 // Hazard Recoginizer Implementation
 //===----------------------------------------------------------------------===//
 
 GCNHazardRecognizer::GCNHazardRecognizer(const MachineFunction &MF) :
   CurrCycleInstr(nullptr),
   MF(MF),
-  ST(MF.getSubtarget<SISubtarget>()) {
+  ST(MF.getSubtarget<SISubtarget>()),
+  TII(*ST.getInstrInfo()) {
   MaxLookAhead = 5;
 }
 
 void GCNHazardRecognizer::EmitInstruction(SUnit *SU) {
   EmitInstruction(SU->getInstr());
 }
 
 void GCNHazardRecognizer::EmitInstruction(MachineInstr *MI) {
static bool isRWLane(unsigned Opcode) {
   return Opcode == AMDGPU::V_READLANE_B32 || Opcode == AMDGPU::V_WRITELANE_B32;
 }
 
 static bool isRFE(unsigned Opcode) {
   return Opcode == AMDGPU::S_RFE_B64;
 }
 
 static bool isSMovRel(unsigned Opcode) {
-  return Opcode == AMDGPU::S_MOVRELS_B32 || AMDGPU::S_MOVRELS_B64 ||
-         Opcode == AMDGPU::S_MOVRELD_B32 || AMDGPU::S_MOVRELD_B64;
+  switch (Opcode) {
+  case AMDGPU::S_MOVRELS_B32:
+  case AMDGPU::S_MOVRELS_B64:
+  case AMDGPU::S_MOVRELD_B32:
+  case AMDGPU::S_MOVRELD_B64:
+    return true;
+  default:
+    return false;
   }
-
-static bool isVInterp(unsigned Opcode) {
-  return Opcode == AMDGPU::V_INTERP_P1_F32 ||
-         Opcode == AMDGPU::V_INTERP_P1_F32_16bank ||
-         Opcode == AMDGPU::V_INTERP_P2_F32 ||
-         Opcode == AMDGPU::V_INTERP_MOV_F32;
 }
 
 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_;
 }
 
if (isSGetReg(MI->getOpcode()) && checkGetRegHazards(MI) > 0)
     return NoopHazard;
 
   if (isSSetReg(MI->getOpcode()) && checkSetRegHazards(MI) > 0)
     return NoopHazard;
 
   if (isRFE(MI->getOpcode()) && checkRFEHazards(MI) > 0)
     return NoopHazard;
 
-  if ((isVInterp(MI->getOpcode()) || isSMovRel(MI->getOpcode())) &&
+  if ((TII.isVINTRP(*MI) || isSMovRel(MI->getOpcode())) &&
       checkReadM0Hazards(MI) > 0)
     return NoopHazard;
 
   if (checkAnyInstHazards(MI) > 0)
     return NoopHazard;
 
   return NoHazard;
 }
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 (isVInterp(MI->getOpcode()))
+    if (TII.isVINTRP(*MI))
       WaitStates = std::max(WaitStates, checkReadM0Hazards(MI));
 
     return WaitStates;
   }
 
   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 (isSMovRel(MI->getOpcode()))
+  if (TII.isVINTRP(*MI) || isSMovRel(MI->getOpcode()))
     return std::max(WaitStates, checkReadM0Hazards(MI));
 
   return WaitStates;
 }
 
 void GCNHazardRecognizer::EmitNoop() {
   EmittedInstrs.push_front(nullptr);
 }
 
 void GCNHazardRecognizer::AdvanceCycle() {
   // When the scheduler detects a stall, it will call AdvanceCycle() without
   // emitting any instructions.
   if (!CurrCycleInstr)
     return;
 
-  const SIInstrInfo *TII = ST.getInstrInfo();
-  unsigned NumWaitStates = TII->getNumWaitStates(*CurrCycleInstr);
+  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.
   for (unsigned i = 1, e = std::min(NumWaitStates, getMaxLookAhead());
int GCNHazardRecognizer::checkSMEMSoftClauseHazards(MachineInstr *SMEM) {
   if (End != Result.begin())
     return 1;
 
   return 0;
 }
 
 int GCNHazardRecognizer::checkSMRDHazards(MachineInstr *SMRD) {
   const SISubtarget &ST = MF.getSubtarget<SISubtarget>();
-  const SIInstrInfo *TII = ST.getInstrInfo();
   int WaitStatesNeeded = 0;
 
   WaitStatesNeeded = checkSMEMSoftClauseHazards(SMRD);
 
   // This SMRD hazard only affects SI.
   if (ST.getGeneration() != SISubtarget::SOUTHERN_ISLANDS)
     return WaitStatesNeeded;
 
   // A read of an SGPR by SMRD instruction requires 4 wait states when the
   // SGPR was written by a VALU instruction.
   int SmrdSgprWaitStates = 4;
-  auto IsHazardDefFn = [TII] (MachineInstr *MI) { return TII->isVALU(*MI); };
+  auto IsHazardDefFn = [this] (MachineInstr *MI) { return TII.isVALU(*MI); };
 
   for (const MachineOperand &Use : SMRD->uses()) {
     if (!Use.isReg())
       continue;
     int WaitStatesNeededForUse =
         SmrdSgprWaitStates - getWaitStatesSinceDef(Use.getReg(), IsHazardDefFn);
     WaitStatesNeeded = std::max(WaitStatesNeeded, WaitStatesNeededForUse);
   }

lib/Target/AMDGPU/SIInstrInfo.h

public:
   static bool isVOP3P(const MachineInstr &MI) {
     return MI.getDesc().TSFlags & SIInstrFlags::VOP3P;
   }
 
   bool isVOP3P(uint16_t Opcode) const {
     return get(Opcode).TSFlags & SIInstrFlags::VOP3P;
   }
 
+  static bool isVINTRP(const MachineInstr &MI) {
+    return MI.getDesc().TSFlags & SIInstrFlags::VINTRP;
+  }
+
+  bool isVINTRP(uint16_t Opcode) const {
+    return get(Opcode).TSFlags & SIInstrFlags::VINTRP;
+  }
+
   static bool isScalarUnit(const MachineInstr &MI) {
     return MI.getDesc().TSFlags & (SIInstrFlags::SALU | SIInstrFlags::SMRD);
   }
 
   static bool usesVM_CNT(const MachineInstr &MI) {
     return MI.getDesc().TSFlags & SIInstrFlags::VM_CNT;
   }