AMDGPU: Force s_waitcnt after GWS instructions

[arsenm]

Summary

This is apparently required to be the immediately following
instruction, so force it into a bundle with a waitcnt.

[rampitec]

Is there any documentation describing it? Also this may be a better job for wait count insertion pass or hazard recognizer, depending on the nature of the issue.

[arsenm]

In D64508#1579479, @rampitec wrote:

Is there any documentation describing it? Also this may be a better job for wait count insertion pass or hazard recognizer, depending on the nature of the issue.

The documentation for these is really not great. It needs to be the literal next instruction, since it will jump back 8 bytes and retry in some situations. I'm not sure why it needs to be a full wait. I didn't want to rely on some other pass later to insert it, since it must be there. Having the waitcnt already exist also avoids teaching the waitcnt pass about this special case, and instead the general pre-existing waitcnt handling will take care of it.

[rampitec]

LGTM

[arsenm]

r366607

[nhaehnle]

My understanding is that this is mostly related to CWSR. The trap handler has to be able to "replay" the GWS instruction.

lib/Target/AMDGPU/DSInstructions.td

: DS_Pseudo<opName, (outs), ins, asmOps> {
 
   let has_gds   = 0;
   let gdsValue  = 1;
   let AsmMatchConverter = "cvtDSGds";
 }
 
 class DS_GWS_0D <string opName>
 : DS_GWS<opName,
-  (ins offset:$offset, gds:$gds), "$offset gds">;
+  (ins offset:$offset, gds:$gds), "$offset gds"> {
+  let hasSideEffects = 1;
+}
 
 class DS_GWS_1D <string opName>
 : DS_GWS<opName,
   (ins VGPR_32:$data0, offset:$offset, gds:$gds), "$data0$offset gds"> {
 
   let has_data0 = 1;
+  let hasSideEffects = 1;
 }
 
 class DS_VOID <string opName> : DS_Pseudo<opName,
   (outs), (ins), ""> {
   let mayLoad = 0;
   let mayStore = 0;
   let hasSideEffects = 1;
   let UseNamedOperandTable = 0;

lib/Target/AMDGPU/SIISelLowering.h

SDValue LowerCall(CallLoweringInfo &CLI,
                     SmallVectorImpl<SDValue> &InVals) const override;
 
   unsigned getRegisterByName(const char* RegName, EVT VT,
                              SelectionDAG &DAG) const override;
 
   MachineBasicBlock *splitKillBlock(MachineInstr &MI,
                                     MachineBasicBlock *BB) const;
 
+  void bundleInstWithWaitcnt(MachineInstr &MI) const;
   MachineBasicBlock *emitGWSMemViolTestLoop(MachineInstr &MI,
                                             MachineBasicBlock *BB) const;
 
   MachineBasicBlock *
   EmitInstrWithCustomInserter(MachineInstr &MI,
                               MachineBasicBlock *BB) const override;
 
   bool hasBitPreservingFPLogic(EVT VT) const override;

lib/Target/AMDGPU/SIISelLowering.cpp

if (InstInLoop) {
     RemainderBB->splice(RemainderBB->begin(), &MBB, I, MBB.end());
   }
 
   MBB.addSuccessor(LoopBB);
 
   return std::make_pair(LoopBB, RemainderBB);
 }
 
+/// Insert \p MI into a BUNDLE with an S_WAITCNT 0 immediately following it.
+void SITargetLowering::bundleInstWithWaitcnt(MachineInstr &MI) const {
+  MachineBasicBlock *MBB = MI.getParent();
+  const SIInstrInfo *TII = getSubtarget()->getInstrInfo();
+  auto I = MI.getIterator();
+  auto E = std::next(I);
+
+  BuildMI(*MBB, E, MI.getDebugLoc(), TII->get(AMDGPU::S_WAITCNT))
+    .addImm(0);
+
+  MIBundleBuilder Bundler(*MBB, I, E);
+  finalizeBundle(*MBB, Bundler.begin());
+}
+
 MachineBasicBlock *
 SITargetLowering::emitGWSMemViolTestLoop(MachineInstr &MI,
                                          MachineBasicBlock *BB) const {
   const DebugLoc &DL = MI.getDebugLoc();
 
   MachineRegisterInfo &MRI = BB->getParent()->getRegInfo();
 
   MachineBasicBlock *LoopBB;
BuildMI(*BB, std::next(Prev), DL, TII->get(AMDGPU::COPY), Data0)
       .add(*Src);
 
     BuildMI(*LoopBB, LoopBB->begin(), DL, TII->get(AMDGPU::COPY), Src->getReg())
       .addReg(Data0);
 
     MRI.setSimpleHint(Data0, Src->getReg());
   }
 
-  BuildMI(*LoopBB, I, DL, TII->get(AMDGPU::S_WAITCNT))
-    .addImm(0);
+  bundleInstWithWaitcnt(MI);
 
   unsigned Reg = MRI.createVirtualRegister(&AMDGPU::SReg_32_XM0RegClass);
 
   // Load and check TRAP_STS.MEM_VIOL
   BuildMI(*LoopBB, I, DL, TII->get(AMDGPU::S_GETREG_B32), Reg)
     .addImm(EncodedReg);
 
   // FIXME: Do we need to use an isel pseudo that may clobber scc?
case AMDGPU::V_SUBREV_I32_e32: {
     return BB;
   }
   case AMDGPU::DS_GWS_INIT:
   case AMDGPU::DS_GWS_SEMA_V:
   case AMDGPU::DS_GWS_SEMA_BR:
   case AMDGPU::DS_GWS_SEMA_P:
   case AMDGPU::DS_GWS_SEMA_RELEASE_ALL:
   case AMDGPU::DS_GWS_BARRIER:
-    if (getSubtarget()->hasGWSAutoReplay())
+    // A s_waitcnt 0 is required to be the instruction immediately following.
+    if (getSubtarget()->hasGWSAutoReplay()) {
+      bundleInstWithWaitcnt(MI);
       return BB;
+    }
+
     return emitGWSMemViolTestLoop(MI, BB);
   default:
     return AMDGPUTargetLowering::EmitInstrWithCustomInserter(MI, BB);
   }
 }
 
 bool SITargetLowering::hasBitPreservingFPLogic(EVT VT) const {
   return isTypeLegal(VT.getScalarType());

lib/Target/AMDGPU/SIInstrInfo.cpp

bool SIInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
   }
   case AMDGPU::EXIT_WWM: {
     // This only gets its own opcode so that SIPreAllocateWWMRegs can tell when
     // WWM is exited.
     MI.setDesc(get(ST.isWave32() ? AMDGPU::S_MOV_B32 : AMDGPU::S_MOV_B64));
     break;
   }
   case TargetOpcode::BUNDLE: {
-    if (!MI.mayLoad())
+    if (!MI.mayLoad() || MI.hasUnmodeledSideEffects())
       return false;
 
     // If it is a load it must be a memory clause
     for (MachineBasicBlock::instr_iterator I = MI.getIterator();
          I->isBundledWithSucc(); ++I) {
       I->unbundleFromSucc();
       for (MachineOperand &MO : I->operands())
         if (MO.isReg())

test/CodeGen/AMDGPU/llvm.amdgcn.ds.gws.barrier.ll

 ; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti -o - -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefixes=GCN,LOOP %s
 ; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=hawaii -o - -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefixes=GCN,LOOP %s
 ; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=fiji -o - -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefixes=GCN,LOOP %s
 ; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx900 -o - -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefixes=GCN,NOLOOP %s
 ; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx1010 -o - -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefixes=GCN,NOLOOP,GFX10 %s
 
+; Make sure the op is emitted bundled with a waitcnt with and without the retry loop, and the bundle is not removed by ExpandPostRAPseudos.
+; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti -stop-after=postrapseudos -o - -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefix=MIR %s
+; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx900 -stop-after=postrapseudos -o - -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefix=MIR %s
+
+
 ; Minimum offset
 ; GCN-LABEL: {{^}}gws_barrier_offset0:
 ; NOLOOP-DAG: s_load_dword [[BAR_NUM:s[0-9]+]]
 ; NOLOOP-DAG: s_mov_b32 m0, -1{{$}}
 ; NOLOOP: v_mov_b32_e32 v0, [[BAR_NUM]]
 ; NOLOOP: ds_gws_barrier v0 offset:1 gds{{$}}
 
 ; LOOP: [[LOOP:BB[0-9]+_[0-9]+]]:
 ; LOOP-NEXT: s_setreg_imm32_b32 hwreg(HW_REG_TRAPSTS, 8, 1), 0
 ; LOOP-NEXT: ds_gws_barrier v0 offset:1 gds
 ; LOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; LOOP-NEXT: s_getreg_b32 [[GETREG:s[0-9]+]], hwreg(HW_REG_TRAPSTS, 8, 1)
 ; LOOP-NEXT: s_cmp_lg_u32 [[GETREG]], 0
 ; LOOP-NEXT: s_cbranch_scc1 [[LOOP]]
+
+; MIR-LABEL: name: gws_barrier_offset0{{$}}
+; MIR: BUNDLE implicit{{( killed)?}} $vgpr0, implicit $m0, implicit $exec {
+; MIR-NEXT: DS_GWS_BARRIER $vgpr0, 1, -1, implicit $m0, implicit $exec :: (load 4 from custom GWSResource)
+; MIR-NEXT: S_WAITCNT 0
+; MIR-NEXT: }
 define amdgpu_kernel void @gws_barrier_offset0(i32 %val) #0 {
   call void @llvm.amdgcn.ds.gws.barrier(i32 %val, i32 0)
   ret void
 }
 
+; MIR-LABEL: name: gws_barrier_offset63{{$}}
+
 ; Maximum offset
 ; GCN-LABEL: {{^}}gws_barrier_offset63:
 ; NOLOOP-DAG: s_load_dword [[BAR_NUM:s[0-9]+]]
 ; NOLOOP-DAG: s_mov_b32 m0, -1{{$}}
 ; NOLOOP-DAG: v_mov_b32_e32 v0, [[BAR_NUM]]
 ; NOLOOP: ds_gws_barrier v0 offset:64 gds{{$}}
 define amdgpu_kernel void @gws_barrier_offset63(i32 %val) #0 {
   call void @llvm.amdgcn.ds.gws.barrier(i32 %val, i32 63)
define amdgpu_kernel void @gws_barrier_save_m0_barrier_constant_offset(i32 %val) #0 {
   call void @llvm.amdgcn.ds.gws.barrier(i32 %val, i32 10)
   store i32 2, i32 addrspace(3)* @lds
   ret void
 }
 
 ; Make sure this increments lgkmcnt
 ; GCN-LABEL: {{^}}gws_barrier_lgkmcnt:
 ; NOLOOP: ds_gws_barrier v0 offset:1 gds{{$}}
-; NOLOOP-NEXT: s_waitcnt expcnt(0) lgkmcnt(0)
+; NOLOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; NOLOOP-NEXT: s_setpc_b64
 define void @gws_barrier_lgkmcnt(i32 %val) {
   call void @llvm.amdgcn.ds.gws.barrier(i32 %val, i32 0)
   ret void
 }
 
 ; Does not imply memory fence on its own
 ; GCN-LABEL: {{^}}gws_barrier_wait_before:
 ; NOLOOP: s_waitcnt
 ; NOLOOP-NOT: s_waitcnt{{$}}
 define amdgpu_kernel void @gws_barrier_wait_before(i32 %val, i32 addrspace(1)* %ptr) #0 {
   store i32 0, i32 addrspace(1)* %ptr
   call void @llvm.amdgcn.ds.gws.barrier(i32 %val, i32 7)
   ret void
 }
 
 ; GCN-LABEL: {{^}}gws_barrier_wait_after:
 ; NOLOOP: ds_gws_barrier v0 offset:8 gds
-; NOLOOP-NEXT: s_waitcnt expcnt(0){{$}}
+; NOLOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; NOLOOP-NEXT: load_dword
 define amdgpu_kernel void @gws_barrier_wait_after(i32 %val, i32 addrspace(1)* %ptr) #0 {
   call void @llvm.amdgcn.ds.gws.barrier(i32 %val, i32 7)
   %load = load volatile i32, i32 addrspace(1)* %ptr
   ret void
 }
 
 ; Does not imply memory fence on its own
 ; GCN-LABEL: {{^}}gws_barrier_fence_before:
 ; NOLOOP: store_dword
 ; NOLOOP: s_waitcnt vmcnt(0) lgkmcnt(0)
 ; NOLOOP: ds_gws_barrier v0 offset:8 gds
+; NOLOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 define amdgpu_kernel void @gws_barrier_fence_before(i32 %val, i32 addrspace(1)* %ptr) #0 {
   store i32 0, i32 addrspace(1)* %ptr
   fence release
   call void @llvm.amdgcn.ds.gws.barrier(i32 %val, i32 7)
   ret void
 }
 
+; FIXME: Extra waitcnt
 ; GCN-LABEL: {{^}}gws_barrier_fence_after:
 ; NOLOOP: ds_gws_barrier v0 offset:8 gds
 ; NOLOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; NOLOOP-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
 ; GFX10-NEXT: s_waitcnt_vscnt null, 0x0
 ; NOLOOP-NEXT: load_dword
 define amdgpu_kernel void @gws_barrier_fence_after(i32 %val, i32 addrspace(1)* %ptr) #0 {
   call void @llvm.amdgcn.ds.gws.barrier(i32 %val, i32 7)
   fence release
   %load = load volatile i32, i32 addrspace(1)* %ptr
   ret void
 }
 
 ; FIXME: Should a wait be inserted here, or is an explicit fence needed?
 ; GCN-LABEL: {{^}}gws_init_barrier:
 ; NOLOOP: s_mov_b32 m0, -1
 ; NOLOOP: ds_gws_init v0 offset:8 gds
+; NOLOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; NOLOOP-NEXT: ds_gws_barrier v0 offset:8 gds
+; NOLOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 define amdgpu_kernel void @gws_init_barrier(i32 %val) #0 {
   call void @llvm.amdgcn.ds.gws.init(i32 %val, i32 7)
   call void @llvm.amdgcn.ds.gws.barrier(i32 %val, i32 7)
   ret void
 }
 
 ; FIXME: Why vmcnt, not expcnt?
 ; GCN-LABEL: {{^}}gws_init_fence_barrier:
 ; NOLOOP: s_mov_b32 m0, -1
 ; NOLOOP: ds_gws_init v0 offset:8 gds
+; NOLOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; NOLOOP-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
 ; GFX10-NEXT: s_waitcnt_vscnt null, 0x0
 ; NOLOOP-NEXT: ds_gws_barrier v0 offset:8 gds
+; NOLOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 define amdgpu_kernel void @gws_init_fence_barrier(i32 %val) #0 {
   call void @llvm.amdgcn.ds.gws.init(i32 %val, i32 7)
   fence release
   call void @llvm.amdgcn.ds.gws.barrier(i32 %val, i32 7)
   ret void
 }
 
 declare void @llvm.amdgcn.ds.gws.barrier(i32, i32) #1
 declare void @llvm.amdgcn.ds.gws.init(i32, i32) #2
 declare i32 @llvm.amdgcn.workitem.id.x() #3
 
 attributes #0 = { nounwind }
 attributes #1 = { convergent inaccessiblememonly nounwind }
 attributes #2 = { convergent inaccessiblememonly nounwind writeonly }
 attributes #3 = { nounwind readnone speculatable }

test/CodeGen/AMDGPU/llvm.amdgcn.ds.gws.init.ll

define amdgpu_kernel void @gws_init_save_m0_init_constant_offset(i32 %val) #0 {
   store i32 1, i32 addrspace(3)* @lds
   call void @llvm.amdgcn.ds.gws.init(i32 %val, i32 10)
   store i32 2, i32 addrspace(3)* @lds
   ret void
 }
 
 ; GCN-LABEL: {{^}}gws_init_lgkmcnt:
 ; NOLOOP: ds_gws_init v0 offset:1 gds{{$}}
-; NOLOOP-NEXT: s_waitcnt expcnt(0) lgkmcnt(0)
+; NOLOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; NOLOOP-NEXT: s_setpc_b64
 define void @gws_init_lgkmcnt(i32 %val) {
   call void @llvm.amdgcn.ds.gws.init(i32 %val, i32 0)
   ret void
 }
 
 ; Does not imply memory fence on its own
 ; GCN-LABEL: {{^}}gws_init_wait_before:
-; NOLOOP: s_waitcnt
+; NOLOOP: s_waitcnt lgkmcnt(0)
 ; NOLOOP-NOT: s_waitcnt
+; NOLOOP: ds_gws_init
+; NOLOOP-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 define amdgpu_kernel void @gws_init_wait_before(i32 %val, i32 addrspace(1)* %ptr) #0 {
   store i32 0, i32 addrspace(1)* %ptr
   call void @llvm.amdgcn.ds.gws.init(i32 %val, i32 7)
   ret void
 }
 
 declare void @llvm.amdgcn.ds.gws.init(i32, i32) #1
 declare i32 @llvm.amdgcn.workitem.id.x() #2
 
 attributes #0 = { nounwind }
 attributes #1 = { convergent inaccessiblememonly nounwind writeonly }
 attributes #2 = { nounwind readnone speculatable }