[AMDGPU] Skip pseudo MIs in hazard recognizer

[cdevadas]

Summary

Instructions like WAVE_BARRIER and SI_MASKED_UNREACHABLE
are only placeholders to prevent certain unwanted
transformations and will get discarded during assembly emission.
They should not be counted during nop insertion.

[arsenm]

I think you're missing the check in AdvanceCycle

[cdevadas]

In D108022#2943730, @arsenm wrote:

I think you're missing the check in AdvanceCycle

There was a call to getNumWaitStates in AdvanceCycle. I presumed a zero waitstate here is equivalent to an early return.
Isn't it?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L365

[arsenm]

In D108022#2943806, @cdevadas wrote:

In D108022#2943730, @arsenm wrote:

I think you're missing the check in AdvanceCycle

There was a call to getNumWaitStates in AdvanceCycle. I presumed a zero waitstate here is equivalent to an early return.
Isn't it?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L365

I think the early return on isMetaInstruction avoids pushing the instruction into EmittedInstrs

[rampitec]

In D108022#2943841, @arsenm wrote:

In D108022#2943806, @cdevadas wrote:

In D108022#2943730, @arsenm wrote:

I think you're missing the check in AdvanceCycle

There was a call to getNumWaitStates in AdvanceCycle. I presumed a zero waitstate here is equivalent to an early return.
Isn't it?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L365

I think the early return on isMetaInstruction avoids pushing the instruction into EmittedInstrs

I think AdvanceCycle shall do the same as with isMetaInstruction() if getNumWaitStates() returned 0.

[cdevadas]

In D108022#2944003, @rampitec wrote:

In D108022#2943841, @arsenm wrote:

In D108022#2943806, @cdevadas wrote:

In D108022#2943730, @arsenm wrote:

I think you're missing the check in AdvanceCycle

There was a call to getNumWaitStates in AdvanceCycle. I presumed a zero waitstate here is equivalent to an early return.
Isn't it?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L365

I think the early return on isMetaInstruction avoids pushing the instruction into EmittedInstrs

I think AdvanceCycle shall do the same as with isMetaInstruction() if getNumWaitStates() returned 0.

The AdvanceCycle will be called by both Post-RA scheduler and hazard recognizer. I am beginning to think that the early return is necessary to prevent adding it into EmittedInstrs.
Alternatively, how about return early when NumWaitStates is zero?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L366

[rampitec]

In D108022#2947383, @cdevadas wrote:

In D108022#2944003, @rampitec wrote:

In D108022#2943841, @arsenm wrote:

In D108022#2943806, @cdevadas wrote:

In D108022#2943730, @arsenm wrote:

I think you're missing the check in AdvanceCycle

There was a call to getNumWaitStates in AdvanceCycle. I presumed a zero waitstate here is equivalent to an early return.
Isn't it?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L365

I think the early return on isMetaInstruction avoids pushing the instruction into EmittedInstrs

I think AdvanceCycle shall do the same as with isMetaInstruction() if getNumWaitStates() returned 0.

The AdvanceCycle will be called by both Post-RA scheduler and hazard recognizer. I am beginning to think that the early return is necessary to prevent adding it into EmittedInstrs.
Alternatively, how about return early when NumWaitStates is zero?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L366

That's what I mean by "I think AdvanceCycle shall do the same as with isMetaInstruction() if getNumWaitStates() returned 0."

[kerbowa]

In D108022#2947383, @cdevadas wrote:

In D108022#2944003, @rampitec wrote:

In D108022#2943841, @arsenm wrote:

In D108022#2943806, @cdevadas wrote:

In D108022#2943730, @arsenm wrote:

I think you're missing the check in AdvanceCycle

There was a call to getNumWaitStates in AdvanceCycle. I presumed a zero waitstate here is equivalent to an early return.
Isn't it?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L365

I think the early return on isMetaInstruction avoids pushing the instruction into EmittedInstrs

I think AdvanceCycle shall do the same as with isMetaInstruction() if getNumWaitStates() returned 0.

The AdvanceCycle will be called by both Post-RA scheduler and hazard recognizer. I am beginning to think that the early return is necessary to prevent adding it into EmittedInstrs.
Alternatively, how about return early when NumWaitStates is zero?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L366

Right, it matters in the post-RA scheduler. For example, if you replace the KILL with WAVE_BARRIER in the test hazard-kill.mir, the scheduler doesn't see the hazard.

[cdevadas]

Added early return from AdvanceCycle if NumWaitStates is zero.
It is required for Post-RA scheduler.

[rampitec]

LGTM, thanks!

llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp

void GCNHazardRecognizer::AdvanceCycle() {
   }
 
   if (CurrCycleInstr->isBundle()) {
     processBundle();
     return;
   }
 
   unsigned NumWaitStates = TII.getNumWaitStates(*CurrCycleInstr);
+  if (!NumWaitStates) {
+    CurrCycleInstr = nullptr;
+    return;
+  }
 
   // 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());

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

 }
 
 unsigned SIInstrInfo::getNumWaitStates(const MachineInstr &MI) {
   switch (MI.getOpcode()) {
   default: return 1; // FIXME: Do wait states equal cycles?
 
   case AMDGPU::S_NOP:
     return MI.getOperand(0).getImm() + 1;
+
+  // FIXME: Any other pseudo instruction?
+  // SI_RETURN_TO_EPILOG is a fallthrough to code outside of the function. The
+  // hazard, even if one exist, won't really be visible. Should we handle it?
+  case AMDGPU::SI_MASKED_UNREACHABLE:
+  case AMDGPU::WAVE_BARRIER:
+    return 0;
   }
 }
 
 bool SIInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
   const SIRegisterInfo *TRI = ST.getRegisterInfo();
   MachineBasicBlock &MBB = *MI.getParent();
   DebugLoc DL = MBB.findDebugLoc(MI);
   switch (MI.getOpcode()) {

llvm/test/CodeGen/AMDGPU/hazard-pseudo-machineinstrs.mir

+# RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -verify-machineinstrs -run-pass post-RA-sched %s -o - | FileCheck -check-prefix=GCN %s
+
+# WAVE_BARRIER and SI_MASKED_UNREACHABLE are not really instructions.
+# To fix the hazard (m0 def followed by V_INTERP), the scheduler
+# should move another instruction into the slot.
+---
+# CHECK-LABEL: name: hazard_wave_barrier
+# CHECK-LABEL: bb.0:
+# GCN: $m0 = S_MOV_B32 killed renamable $sgpr0
+# GCN-NEXT: WAVE_BARRIER
+# GCN-NEXT: S_MOV_B32 0
+# GCN-NEXT: V_INTERP_MOV_F32
+name: hazard_wave_barrier
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $sgpr0
+
+    $m0 = S_MOV_B32 killed renamable $sgpr0
+    WAVE_BARRIER
+    renamable $vgpr0 = V_INTERP_MOV_F32 2, 0, 0, implicit $mode, implicit $m0, implicit $exec
+    renamable $sgpr1 = S_MOV_B32 0
+    S_ENDPGM 0
+
+...
+# GCN-LABEL: name: hazard-masked-unreachable
+# CHECK-LABEL: bb.0:
+# GCN: $m0 = S_MOV_B32 killed renamable $sgpr0
+# GCN-NEXT: SI_MASKED_UNREACHABLE
+# GCN-NEXT: S_MOV_B32 0
+# GCN-NEXT: V_INTERP_MOV_F32
+---
+name: hazard-masked-unreachable
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $sgpr0
+
+    $m0 = S_MOV_B32 killed renamable $sgpr0
+    SI_MASKED_UNREACHABLE
+    renamable $vgpr0 = V_INTERP_MOV_F32 2, 0, 0, implicit $mode, implicit $m0, implicit $exec
+    renamable $sgpr1 = S_MOV_B32 0
+  bb.1:
+    S_ENDPGM 0
+...

llvm/test/CodeGen/AMDGPU/hazard.mir

bb.0:
     DBG_LABEL 2
     DBG_LABEL 3
     DBG_LABEL 4
     DBG_LABEL 5
     DBG_LABEL 6
     S_SENDMSG 3, implicit $exec, implicit $m0
     S_ENDPGM 0
 ...
+# GCN-LABEL: name: hazard-lookahead-wave-barrier
+# GCN: S_WAITCNT 0
+# GCN-NEXT: S_NOP 0
+# GCN-NEXT: V_ADD_F16_dpp
+---
+name: hazard-lookahead-wave-barrier
+body: |
+  bb.0:
+    liveins: $vgpr0, $vgpr1, $vgpr3
+
+    renamable $vgpr1 = contract nofpexcept V_ADD_F16_e32 killed $vgpr1, $vgpr0, implicit $mode, implicit $exec
+    WAVE_BARRIER
+    S_WAITCNT 0
+    renamable $vgpr2 = contract nofpexcept V_ADD_F16_dpp undef $vgpr2, 0, $vgpr1, 0, $vgpr3, 273, 15, 15, 1, implicit $mode, implicit $exec
+...
+# GCN-LABEL: name: hazard-lookahead-masked-unreachable
+# GCN: SI_MASKED_UNREACHABLE
+# GCN-NEXT: S_NOP 0
+# GCN-NEXT: S_SENDMSG
+---
+name: hazard-lookahead-masked-unreachable
+body: |
+  bb.0:
+    $m0 = S_MOV_B32 -1
+    SI_MASKED_UNREACHABLE
+    S_SENDMSG 3, implicit $exec, implicit $m0
+
+  bb.1:
+    S_ENDPGM 0
+...
+# GCN-LABEL: name: fallthrough-hazard-lookahead-masked-unreachable
+# GCN: SI_MASKED_UNREACHABLE
+# GCN-LABEL: bb.1:
+# GCN-NEXT: S_NOP 0
+# GCN-NEXT: S_SENDMSG
+---
+name: fallthrough-hazard-lookahead-masked-unreachable
+body: |
+  bb.0:
+    $m0 = S_MOV_B32 -1
+    SI_MASKED_UNREACHABLE
+
+  bb.1:
+    S_SENDMSG 3, implicit $exec, implicit $m0
+    S_ENDPGM 0
+...