AMDGPU: Handle s_buffer_load_dword hazard on SI

[nhaehnle]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

347–374 ↗

(On Diff #119799)

This could be merged with the previous loop, right?

test/CodeGen/AMDGPU/smrd.ll

93 ↗

(On Diff #119799)

Please add a NOTSI-NOT-NEXT line as well (or CIVIGFX9-NOT-NEXT?)

[arsenm]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

347 ↗

(On Diff #119799)

Do you mean just SI? Needs a subtarget check?

354 ↗

(On Diff #119799)

Probably should avoid switch with something like isSMRD() && has buffer operand

[mareko]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

347 ↗

(On Diff #119799)

This code is only executed on SI.

354 ↗

(On Diff #119799)

Not sure if there is a way to check if an opcode has a buffer operand. It looks like not.

test/CodeGen/AMDGPU/smrd.ll

93 ↗

(On Diff #119799)

I think GCN-NEXT already ensures that there can't be any s_nop.

[mareko]

Cleanups.

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

int GCNHazardRecognizer::checkSMRDHazards(MachineInstr *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 = [this] (MachineInstr *MI) { return TII.isVALU(*MI); };
+  auto IsBufferHazardDefFn = [this] (MachineInstr *MI) { return TII.isSALU(*MI); };
+
+  bool IsBufferSMRD = SMRD->getOpcode() == AMDGPU::S_BUFFER_LOAD_DWORD_IMM ||
+                      SMRD->getOpcode() == AMDGPU::S_BUFFER_LOAD_DWORDX2_IMM ||
+                      SMRD->getOpcode() == AMDGPU::S_BUFFER_LOAD_DWORDX4_IMM ||
+                      SMRD->getOpcode() == AMDGPU::S_BUFFER_LOAD_DWORDX8_IMM ||
+                      SMRD->getOpcode() == AMDGPU::S_BUFFER_LOAD_DWORDX16_IMM ||
+                      SMRD->getOpcode() == AMDGPU::S_BUFFER_LOAD_DWORD_SGPR ||
+                      SMRD->getOpcode() == AMDGPU::S_BUFFER_LOAD_DWORDX2_SGPR ||
+                      SMRD->getOpcode() == AMDGPU::S_BUFFER_LOAD_DWORDX4_SGPR ||
+                      SMRD->getOpcode() == AMDGPU::S_BUFFER_LOAD_DWORDX8_SGPR ||
+                      SMRD->getOpcode() == AMDGPU::S_BUFFER_LOAD_DWORDX16_SGPR;
 
   for (const MachineOperand &Use : SMRD->uses()) {
     if (!Use.isReg())
       continue;
     int WaitStatesNeededForUse =
         SmrdSgprWaitStates - getWaitStatesSinceDef(Use.getReg(), IsHazardDefFn);
     WaitStatesNeeded = std::max(WaitStatesNeeded, WaitStatesNeededForUse);
+
+    // This fixes what appears to be undocumented hardware behavior in SI where
+    // s_mov writing a descriptor and s_buffer_load_dword reading the descriptor
+    // needs some number of nops in between. We don't know how many we need, but
+    // let's use 4. This wasn't discovered before probably because the only
+    // case when this happens is when we expand a 64-bit pointer into a full
+    // descriptor and use s_buffer_load_dword instead of s_load_dword, which was
+    // probably never encountered in the closed-source land.
+    if (IsBufferSMRD) {
+      int WaitStatesNeededForUse =
+        SmrdSgprWaitStates - getWaitStatesSinceDef(Use.getReg(),
+                                                   IsBufferHazardDefFn);
+      WaitStatesNeeded = std::max(WaitStatesNeeded, WaitStatesNeededForUse);
+    }
   }
+
   return WaitStatesNeeded;
 }
 
 int GCNHazardRecognizer::checkVMEMHazards(MachineInstr* VMEM) {
   const SIInstrInfo *TII = ST.getInstrInfo();
 
   if (ST.getGeneration() < SISubtarget::VOLCANIC_ISLANDS)
     return 0;

llvm/trunk/test/CodeGen/AMDGPU/smrd.ll

 define amdgpu_kernel void @smrd5(i32 addrspace(1)* %out, i32 addrspace(2)* %ptr) #0 {
 entry:
   %tmp = getelementptr i32, i32 addrspace(2)* %ptr, i64 262144
   %tmp1 = load i32, i32 addrspace(2)* %tmp
   store i32 %tmp1, i32 addrspace(1)* %out
   ret void
 }
 
+; GCN-LABEL: {{^}}smrd_hazard:
+; GCN-DAG: s_mov_b32 s3, 3
+; GCN-DAG: s_mov_b32 s2, 2
+; GCN-DAG: s_mov_b32 s1, 1
+; GCN-DAG: s_mov_b32 s0, 0
+; SI-NEXT: nop 3
+; GCN-NEXT: s_buffer_load_dword s0, s[0:3], 0x0
+define amdgpu_ps float @smrd_hazard(<4 x i32> inreg %desc) #0 {
+main_body:
+  %d0 = insertelement <4 x i32> undef, i32 0, i32 0
+  %d1 = insertelement <4 x i32> %d0, i32 1, i32 1
+  %d2 = insertelement <4 x i32> %d1, i32 2, i32 2
+  %d3 = insertelement <4 x i32> %d2, i32 3, i32 3
+  %r = call float @llvm.SI.load.const.v4i32(<4 x i32> %d3, i32 0)
+  ret float %r
+}
+
 ; SMRD load using the load.const.v4i32 intrinsic with an immediate offset
 ; GCN-LABEL: {{^}}smrd_load_const0:
 ; SICI: s_buffer_load_dword s{{[0-9]}}, s[{{[0-9]:[0-9]}}], 0x4 ; encoding: [0x04
 ; VI: s_buffer_load_dword s{{[0-9]}}, s[{{[0-9]:[0-9]}}], 0x10
 define amdgpu_ps void @smrd_load_const0(<4 x i32> addrspace(2)* inreg %arg, <4 x i32> addrspace(2)* inreg %arg1, <32 x i8> addrspace(2)* inreg %arg2, i32 inreg %arg3, <2 x i32> %arg4, <2 x i32> %arg5, <2 x i32> %arg6, <3 x i32> %arg7, <2 x i32> %arg8, <2 x i32> %arg9, <2 x i32> %arg10, float %arg11, float %arg12, float %arg13, float %arg14, float %arg15, float %arg16, float %arg17, float %arg18, float %arg19) #0 {
 main_body:
   %tmp = getelementptr <4 x i32>, <4 x i32> addrspace(2)* %arg, i32 0
   %tmp20 = load <4 x i32>, <4 x i32> addrspace(2)* %tmp