[AMDGPU] Remove non-instructions from GCNHazardRecognizer buffer

[critson]

Summary

This fixes a bug where a large number of implicit def instructions can cause the GCNHazardRecognizer not to insert required NOPs.

Change-Id: Ie75338f94de704ee5816b05afd0c922c6748a95b

[arsenm]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

222 ↗

(On Diff #164051)

isDebugInstr()

test/CodeGen/AMDGPU/hazard-lookahead.mir

8–13 ↗

(On Diff #164051)

We already have a test for implicit_def behavior in hazard.mir? You can merge this test into that one

38 ↗

(On Diff #164051)

Need another test with DBG_LABEL

[arsenm]

test/CodeGen/AMDGPU/hazard-lookahead.mir

12–13 ↗

(On Diff #164051)

Should also use -NEXT checks

[critson]

Use isDebugInstr instead of DEBUG_VALUE.
Merge tests into existing hazard.mir.
Add test for DEBUG_LABEL.

[arsenm]

LGTM, but reword the commit message since nothing is really removed

[nhaehnle]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

263–265 ↗

(On Diff #164385)

Please remove INLINEASM here, as well. A test case would be good as well.

There is no guarantee that inline assembly contains actual instructions, and in fact there are cases where Mesa currently generates fake inline assembly as a workaround.

[nhaehnle]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

263–265 ↗

(On Diff #164385)

Actually, scratch that, I got my wires crossed about the logic of this code. This part of it should be fine?

However, I think the point stands that the same issue that affects DBG_VALUEs with respect to the buffer can also affect inline assembly instructions.

[critson]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

263–265 ↗

(On Diff #164385)

We cannot remove INLINEASM from the buffer as INLINEASM can interact with registers and cause the introduction of NOPs.
There is a test for this behaviour in hazard-inlineasm.mir.
If a large number of INLINEASM are overflowing the buffer then this will require a separate workaround (or increased buffer size).

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

 }
 
 void GCNHazardRecognizer::AdvanceCycle() {
   // When the scheduler detects a stall, it will call AdvanceCycle() without
   // emitting any instructions.
   if (!CurrCycleInstr)
     return;
 
+  // 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->getOpcode() == AMDGPU::IMPLICIT_DEF)
+    return;
+  else if (CurrCycleInstr->isDebugInstr())
+    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.
int GCNHazardRecognizer::getWaitStatesSince(
     function_ref<bool(MachineInstr *)> IsHazard) {
   int WaitStates = 0;
   for (MachineInstr *MI : EmittedInstrs) {
     if (MI) {
       if (IsHazard(MI))
         return WaitStates;
 
       unsigned Opcode = MI->getOpcode();
-      if (Opcode == AMDGPU::DBG_VALUE || Opcode == AMDGPU::IMPLICIT_DEF ||
-          Opcode == AMDGPU::INLINEASM)
+      if (Opcode == AMDGPU::INLINEASM)
         continue;
     }
     ++WaitStates;
   }
   return std::numeric_limits<int>::max();
 }
 
 int GCNHazardRecognizer::getWaitStatesSinceDef(

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

 # RUN: llc -march=amdgcn -mcpu=tonga -verify-machineinstrs -run-pass post-RA-hazard-rec %s -o - | FileCheck -check-prefix=GCN -check-prefix=VI %s
+# RUN: llc -march=amdgcn -mcpu=gfx803 -verify-machineinstrs -run-pass post-RA-hazard-rec %s -o - | FileCheck -check-prefix=GCN -check-prefix=GFX8 %s
 # RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -run-pass post-RA-hazard-rec %s -o - | FileCheck -check-prefix=GCN -check-prefix=GFX9 %s
 
 # GCN-LABEL: name: hazard_implicit_def
 # GCN:    bb.0.entry:
 # GCN:      $m0 = S_MOV_B32
 # GFX9:     S_NOP 0
 # VI-NOT:   S_NOP_0
 # GCN:      V_INTERP_P1_F32
body:             |
   bb.0.entry:
     liveins: $sgpr7, $vgpr4
 
     $m0 = S_MOV_B32 killed $sgpr7
     INLINEASM &"; no-op", 1, 327690, def $vgpr5
     $vgpr0 = V_INTERP_P1_F32 killed $vgpr4, 0, 0, implicit $m0, implicit $exec
     SI_RETURN_TO_EPILOG killed $vgpr5, killed $vgpr0
 ...
+
+# IMPLICIT_DEF/DBG_VALUE instructions should not prevent the hazard recognizer
+# from adding s_nop instructions between m0 update and s_sendmsg.
+
+# GCN-LABEL: name: hazard-lookahead-implicit-def
+# GCN: $vgpr6 = IMPLICIT_DEF
+# GFX8-NEXT: S_NOP 0
+# GFX9-NEXT: S_NOP 0
+# GCN: S_SENDMSG 3, implicit $exec, implicit $m0
+---
+name: hazard-lookahead-implicit-def
+body: |
+  bb.0:
+    $m0 = S_MOV_B32 killed $sgpr12
+    $vgpr0 = IMPLICIT_DEF
+    $vgpr1 = IMPLICIT_DEF
+    $vgpr2 = IMPLICIT_DEF
+    $vgpr3 = IMPLICIT_DEF
+    $vgpr4 = IMPLICIT_DEF
+    $vgpr5 = IMPLICIT_DEF
+    $vgpr6 = IMPLICIT_DEF
+    S_SENDMSG 3, implicit $exec, implicit $m0
+    S_ENDPGM
+...
+
+# GCN-LABEL: name: hazard-lookahead-dbg-value
+# GCN: DBG_VALUE 6
+# GFX8-NEXT: S_NOP 0
+# GFX9-NEXT: S_NOP 0
+# GCN: S_SENDMSG 3, implicit $exec, implicit $m0
+---
+name: hazard-lookahead-dbg-value
+body: |
+  bb.0:
+    $m0 = S_MOV_B32 killed $sgpr12
+    DBG_VALUE 0
+    DBG_VALUE 1
+    DBG_VALUE 2
+    DBG_VALUE 3
+    DBG_VALUE 4
+    DBG_VALUE 5
+    DBG_VALUE 6
+    S_SENDMSG 3, implicit $exec, implicit $m0
+    S_ENDPGM
+...
+
+# GCN-LABEL: name: hazard-lookahead-dbg-label
+# GCN: DBG_LABEL 6
+# GFX8-NEXT: S_NOP 0
+# GFX9-NEXT: S_NOP 0
+# GCN: S_SENDMSG 3, implicit $exec, implicit $m0
+---
+name: hazard-lookahead-dbg-label
+body: |
+  bb.0:
+    $m0 = S_MOV_B32 killed $sgpr12
+    DBG_LABEL 0
+    DBG_LABEL 1
+    DBG_LABEL 2
+    DBG_LABEL 3
+    DBG_LABEL 4
+    DBG_LABEL 5
+    DBG_LABEL 6
+    S_SENDMSG 3, implicit $exec, implicit $m0
+    S_ENDPGM
+...