[AMDGPU] run post-RA hazard recognizer pass late

[msearles]

Summary

Memory legalizer, waitcnt, and shrink passes can perturb the instructions, which means that the post-RA hazard recognizer pass should run after them. Otherwise, one of those passes may invalidate the work done by the hazard recognizer. Note that this has adverse side-effect that any consecutive S_NOP 0's, emitted by the hazard recognizer, will not be shrunk into a single S_NOP <N>. This should be addressed in a follow-on patch.

[arsenm]

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

902 ↗

(On Diff #155348)

I'm surprised we run shrink after this point.

I think this can plausibly be an issue.
Suppose you had something like
%vgpr0 = V_FOO
<some hazard>
%vgpr1 = V_MOV_B32 5
%vgpr2 = V_BAR %vgpr1

The immediate materialize would eliminate a wait state that wasn't necessary, but now is since the copy is eliminated. I think in practice the shrink pass isn't very good at folding immediates post-RA, if it even tries

[msearles]

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

902 ↗

(On Diff #155348)

I will move the hazard recognizer after shrink instructions, unless you're suggesting that the shrink pass be moved somewhere else? I think that moving the hazard recognizer after the shrink pass has the side-effect that some S_NOP 0's won't be collapsed into, say S_NOP <N>. The hazard recognizer just creates individual S_NOP 0 instrs and is relying on the shrink pass to turn them into S_NOP <N>. I suppose the hazard recognizer could be enhanced to generate the S_NOP <N> instrs on its own.

[arsenm]

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

902 ↗

(On Diff #155348)

Yes, the hazard recognizer ideally would do that. The shrink pass deals with this as a workaround for it seeming to assume all nops are the same.

It's probably OK to leave as-is for now

[rampitec]

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

902 ↗

(On Diff #155348)

I would move shrink before hazard recognizer.

[msearles]

• Move post-RA hazard recognizer pass after shrink instr as well.
• Update tests.

[rampitec]

LGTM

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

void GCNPassConfig::addPostRegAlloc() {
   addPass(&SIOptimizeExecMaskingID);
   TargetPassConfig::addPostRegAlloc();
 }
 
 void GCNPassConfig::addPreSched2() {
 }
 
 void GCNPassConfig::addPreEmitPass() {
+  addPass(createSIMemoryLegalizerPass());
+  addPass(createSIInsertWaitcntsPass());
+  addPass(createSIShrinkInstructionsPass());
+
   // The hazard recognizer that runs as part of the post-ra scheduler does not
   // guarantee to be able handle all hazards correctly. This is because if there
   // are multiple scheduling regions in a basic block, the regions are scheduled
   // bottom up, so when we begin to schedule a region we don't know what
   // instructions were emitted directly before it.
   //
   // Here we add a stand-alone hazard recognizer pass which can handle all
   // cases.
+  //
+  // FIXME: This stand-alone pass will emit indiv. S_NOP 0, as needed. It would
+  // be better for it to emit S_NOP <N> when possible.
   addPass(&PostRAHazardRecognizerID);
 
-  addPass(createSIMemoryLegalizerPass());
-  addPass(createSIInsertWaitcntsPass());
-  addPass(createSIShrinkInstructionsPass());
   addPass(&SIInsertSkipsPassID);
   addPass(createSIDebuggerInsertNopsPass());
   addPass(&BranchRelaxationPassID);
 }
 
 TargetPassConfig *GCNTargetMachine::createPassConfig(PassManagerBase &PM) {
   return new GCNPassConfig(*this, PM);
 }

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

if ((AddrSpace & SIAtomicAddrSpace::GLOBAL) != SIAtomicAddrSpace::NONE) {
     case SIAtomicScope::SYSTEM:
     case SIAtomicScope::AGENT:
       VMCnt = true;
       break;
     case SIAtomicScope::WORKGROUP:
     case SIAtomicScope::WAVEFRONT:
     case SIAtomicScope::SINGLETHREAD:
       // The L1 cache keeps all memory operations in order for
-      // wavesfronts in the same work-group.
+      // wavefronts in the same work-group.
       break;
     default:
       llvm_unreachable("Unsupported synchronization scope");
     }
   }
 
   if ((AddrSpace & SIAtomicAddrSpace::LDS) != SIAtomicAddrSpace::NONE) {
     switch (Scope) {

llvm/trunk/test/CodeGen/AMDGPU/llvm.amdgcn.mov.dpp.ll

 ; RUN: llc -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs -show-mc-encoding < %s | FileCheck -check-prefix=VI -check-prefix=VI-OPT %s
 ; RUN: llc -O0 -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs -show-mc-encoding < %s | FileCheck -check-prefix=VI -check-prefix=VI-NOOPT %s
 
 ; FIXME: The register allocator / scheduler should be able to avoid these hazards.
 
 ; VI-LABEL: {{^}}dpp_test:
 ; VI: v_mov_b32_e32 v0, s{{[0-9]+}}
 ; VI-NOOPT: v_mov_b32_e32 v1, s{{[0-9]+}}
-; VI: s_nop 1
+; VI-OPT: s_nop 1
+; VI-NOOPT: s_nop 0
+; VI-NOOPT: s_nop 0
 ; VI-OPT: v_mov_b32_dpp v0, v0 quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:0 ; encoding: [0xfa,0x02,0x00,0x7e,0x00,0x01,0x08,0x11]
 ; VI-NOOPT: v_mov_b32_dpp v0, v1 quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:0 ; encoding: [0xfa,0x02,0x00,0x7e,0x01,0x01,0x08,0x11]
 define amdgpu_kernel void @dpp_test(i32 addrspace(1)* %out, i32 %in) {
   %tmp0 = call i32 @llvm.amdgcn.mov.dpp.i32(i32 %in, i32 1, i32 1, i32 1, i1 1) #0
   store i32 %tmp0, i32 addrspace(1)* %out
   ret void
 }
 
 ; VI-LABEL: {{^}}dpp_wait_states:
 ; VI-NOOPT: v_mov_b32_e32 [[VGPR1:v[0-9]+]], s{{[0-9]+}}
 ; VI: v_mov_b32_e32 [[VGPR0:v[0-9]+]], s{{[0-9]+}}
-; VI: s_nop 1
+; VI-OPT: s_nop 1
+; VI-NOOPT: s_nop 0
+; VI-NOOPT: s_nop 0
 ; VI-OPT: v_mov_b32_dpp [[VGPR0]], [[VGPR0]] quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:0
-; VI-NOOPT: v_mov_b32_dpp [[VGPR1]], [[VGPR0]] quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:0
-; VI: s_nop 1
+; VI-NOOPT: v_mov_b32_dpp [[VGPR1]], [[VGPR0]] quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:
+; VI-OPT: s_nop 1
+; VI-NOOPT: s_nop 0
+; VI-NOOPT: s_nop 0
 ; VI-OPT: v_mov_b32_dpp v{{[0-9]+}}, [[VGPR0]] quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:0
 ; VI-NOOPT: v_mov_b32_dpp v{{[0-9]+}}, [[VGPR1]] quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:0
 define amdgpu_kernel void @dpp_wait_states(i32 addrspace(1)* %out, i32 %in) {
   %tmp0 = call i32 @llvm.amdgcn.mov.dpp.i32(i32 %in, i32 1, i32 1, i32 1, i1 1) #0
   %tmp1 = call i32 @llvm.amdgcn.mov.dpp.i32(i32 %tmp0, i32 1, i32 1, i32 1, i1 1) #0
   store i32 %tmp1, i32 addrspace(1)* %out
   ret void
 }
 
 ; VI-LABEL: {{^}}dpp_first_in_bb:
 ; VI: ; %endif
 ; VI-OPT: s_mov_b32
 ; VI-OPT: s_mov_b32
-; VI-NOOPT: s_nop 1
+; VI-NOOPT: s_waitcnt
+; VI-NOOPT-NEXT: s_nop 0
 ; VI: v_mov_b32_dpp [[VGPR0:v[0-9]+]], v{{[0-9]+}} quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:0
-; VI: s_nop 1
+; VI-OPT: s_nop 1
 ; VI: v_mov_b32_dpp [[VGPR1:v[0-9]+]], [[VGPR0]] quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:0
-; VI: s_nop 1
+; VI-OPT: s_nop 1
+; VI-NOOPT: s_nop 0
+; VI-NOOPT: s_nop 0
 ; VI: v_mov_b32_dpp v{{[0-9]+}}, [[VGPR1]] quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:0
 define amdgpu_kernel void @dpp_first_in_bb(float addrspace(1)* %out, float addrspace(1)* %in, float %cond, float %a, float %b) {
   %cmp = fcmp oeq float %cond, 0.0
   br i1 %cmp, label %if, label %else
 
 if:
   %out_val = load float, float addrspace(1)* %out
   %if_val = fadd float %a, %out_val

llvm/trunk/test/CodeGen/AMDGPU/llvm.amdgcn.update.dpp.ll

 ; RUN: llc -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs -show-mc-encoding < %s | FileCheck -check-prefix=VI -check-prefix=VI-OPT %s
 ; RUN: llc -O0 -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs -show-mc-encoding < %s | FileCheck -check-prefix=VI -check-prefix=VI-NOOPT %s
 
 ; VI-LABEL: {{^}}dpp_test:
 ; VI: v_mov_b32_e32 v0, s{{[0-9]+}}
 ; VI: v_mov_b32_e32 v1, s{{[0-9]+}}
-; VI: s_nop 1
+; VI-OPT: s_nop 1
+; VI-NOOPT: s_nop 0
+; VI-NOOPT: s_nop 0
 ; VI: v_mov_b32_dpp v0, v1 quad_perm:[1,0,0,0] row_mask:0x1 bank_mask:0x1 bound_ctrl:0 ; encoding: [0xfa,0x02,0x00,0x7e,0x01,0x01,0x08,0x11]
 define amdgpu_kernel void @dpp_test(i32 addrspace(1)* %out, i32 %in1, i32 %in2) {
   %tmp0 = call i32 @llvm.amdgcn.update.dpp.i32(i32 %in1, i32 %in2, i32 1, i32 1, i32 1, i1 1) #0
   store i32 %tmp0, i32 addrspace(1)* %out
   ret void
 }
 
+; VI-LABEL: {{^}}dpp_test1:
+; VI-OPT: v_add_u32_e32 [[REG:v[0-9]+]], vcc, v{{[0-9]+}}, v{{[0-9]+}}
+; VI-NOOPT: v_mov_b32_e32 v{{[0-9]+}}, 0
+; VI-NOOPT: v_mov_b32_e32 [[REG:v[0-9]+]], v{{[0-9]+}}
+; VI-NEXT: s_nop 0
+; VI-NEXT: s_nop 0
+; VI-NEXT: v_mov_b32_dpp v2, [[REG]] quad_perm:[1,0,3,2] row_mask:0xf bank_mask:0xf
+@0 = internal unnamed_addr addrspace(3) global [448 x i32] undef, align 4
+define weak_odr amdgpu_kernel void @dpp_test1(i32* %arg) local_unnamed_addr {
+bb:
+  %tmp = tail call i32 @llvm.amdgcn.workitem.id.x()
+  %tmp1 = zext i32 %tmp to i64
+  %tmp2 = getelementptr inbounds [448 x i32], [448 x i32] addrspace(3)* @0, i32 0, i32 %tmp
+  %tmp3 = load i32, i32 addrspace(3)* %tmp2, align 4
+  fence syncscope("workgroup") release
+  tail call void @llvm.amdgcn.s.barrier()
+  fence syncscope("workgroup") acquire
+  %tmp4 = add nsw i32 %tmp3, %tmp3
+  %tmp5 = tail call i32 @llvm.amdgcn.update.dpp.i32(i32 0, i32 %tmp4, i32 177, i32 15, i32 15, i1 zeroext false)
+  %tmp6 = add nsw i32 %tmp5, %tmp4
+  %tmp7 = getelementptr inbounds i32, i32* %arg, i64 %tmp1
+  store i32 %tmp6, i32* %tmp7, align 4
+  ret void
+}
+
+declare i32 @llvm.amdgcn.workitem.id.x()
+declare void @llvm.amdgcn.s.barrier()
 declare i32 @llvm.amdgcn.update.dpp.i32(i32, i32, i32, i32, i32, i1) #0
 
 attributes #0 = { nounwind readnone convergent }

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

 ; GCN-NEXT: buffer_load_dword
 ; GCN-NEXT: buffer_load_dword
 ; GCN-NEXT: buffer_load_dword
 ; GCN-NEXT: buffer_load_dword
 ; GCN-NEXT: buffer_load_dword
 ; GCN-NEXT: buffer_load_dword
 ; GCN-NEXT: buffer_load_dword
 ; GCN-NEXT: s_nop
+; GCN-NEXT: s_nop
 ; GCN-NEXT: buffer_load_dword
 define void @mubuf_clause(<4 x i32> addrspace(5)* noalias nocapture readonly %arg, <4 x i32> addrspace(5)* noalias nocapture %arg1) {
 bb:
   %tmp = tail call i32 @llvm.amdgcn.workitem.id.x()
   %tmp2 = getelementptr inbounds <4 x i32>, <4 x i32> addrspace(5)* %arg, i32 %tmp
   %tmp3 = load <4 x i32>, <4 x i32> addrspace(5)* %tmp2, align 16
   %tmp4 = getelementptr inbounds <4 x i32>, <4 x i32> addrspace(5)* %arg1, i32 %tmp
   %tmp5 = add nuw nsw i32 %tmp, 1
bb:
   store <4 x i32> %tmp7, <4 x i32> addrspace(5)* %tmp8, align 16
   store <4 x i32> %tmp11, <4 x i32> addrspace(5)* %tmp12, align 16
   store <4 x i32> %tmp15, <4 x i32> addrspace(5)* %tmp16, align 16
   ret void
 }
 
 ; GCN-LABEL: {{^}}vector_clause_indirect:
 ; GCN: global_load_dwordx2 [[ADDR:v\[[0-9:]+\]]], v[{{[0-9:]+}}], off
-; GCN-NEXT: s_nop
+; GCN-NEXT: s_nop 0
 ; GCN-NEXT: s_waitcnt vmcnt(0)
+; GCN-NEXT: s_nop 0
 ; GCN-NEXT: global_load_dwordx4 v[{{[0-9:]+}}], [[ADDR]], off
 ; GCN-NEXT: global_load_dwordx4 v[{{[0-9:]+}}], [[ADDR]], off offset:16
 define amdgpu_kernel void @vector_clause_indirect(i64 addrspace(1)* noalias nocapture readonly %arg, <4 x i32> addrspace(1)* noalias nocapture readnone %arg1, <4 x i32> addrspace(1)* noalias nocapture %arg2) {
 bb:
   %tmp = tail call i32 @llvm.amdgcn.workitem.id.x()
   %tmp3 = zext i32 %tmp to i64
   %tmp4 = getelementptr inbounds i64, i64 addrspace(1)* %arg, i64 %tmp3
   %tmp5 = bitcast i64 addrspace(1)* %tmp4 to <4 x i32> addrspace(1)* addrspace(1)*
   %tmp6 = load <4 x i32> addrspace(1)*, <4 x i32> addrspace(1)* addrspace(1)* %tmp5, align 8
   %tmp7 = load <4 x i32>, <4 x i32> addrspace(1)* %tmp6, align 16
   %tmp8 = getelementptr inbounds <4 x i32>, <4 x i32> addrspace(1)* %tmp6, i64 1
   %tmp9 = load <4 x i32>, <4 x i32> addrspace(1)* %tmp8, align 16
   store <4 x i32> %tmp7, <4 x i32> addrspace(1)* %arg2, align 16
   %tmp10 = getelementptr inbounds <4 x i32>, <4 x i32> addrspace(1)* %arg2, i64 1
   store <4 x i32> %tmp9, <4 x i32> addrspace(1)* %tmp10, align 16
   ret void
 }
 
 ; GCN-LABEL: {{^}}load_global_d16_hi:
 ; GCN:      global_load_short_d16_hi v
 ; GCN-NEXT: s_nop
+; GCN-NEXT: s_nop
 ; GCN-NEXT: global_load_short_d16_hi v
 define void @load_global_d16_hi(i16 addrspace(1)* %in, i16 %reg, <2 x i16> addrspace(1)* %out) {
 entry:
   %gep = getelementptr inbounds i16, i16 addrspace(1)* %in, i64 32
   %load1 = load i16, i16 addrspace(1)* %in
   %load2 = load i16, i16 addrspace(1)* %gep
   %build0 = insertelement <2 x i16> undef, i16 %reg, i32 0
   %build1 = insertelement <2 x i16> %build0, i16 %load1, i32 1
   store <2 x i16> %build1, <2 x i16> addrspace(1)* %out
   %build2 = insertelement <2 x i16> undef, i16 %reg, i32 0
   %build3 = insertelement <2 x i16> %build2, i16 %load2, i32 1
   %gep2 = getelementptr inbounds <2 x i16>, <2 x i16> addrspace(1)* %out, i64 32
   store <2 x i16> %build3, <2 x i16> addrspace(1)* %gep2
   ret void
 }
 
 ; GCN-LABEL: {{^}}load_global_d16_lo:
 ; GCN:      global_load_short_d16 v
 ; GCN-NEXT: s_nop
+; GCN-NEXT: s_nop
 ; GCN-NEXT: global_load_short_d16 v
 define void @load_global_d16_lo(i16 addrspace(1)* %in, i32 %reg, <2 x i16> addrspace(1)* %out) {
 entry:
   %gep = getelementptr inbounds i16, i16 addrspace(1)* %in, i64 32
   %reg.bc1 = bitcast i32 %reg to <2 x i16>
   %reg.bc2 = bitcast i32 %reg to <2 x i16>
   %load1 = load i16, i16 addrspace(1)* %in
   %load2 = load i16, i16 addrspace(1)* %gep