[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/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?
+  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.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
+...