[AMDGPU] Skip invariant loads when avoiding WAR conflicts

[piotr]

Summary

No need to handle invariant loads when avoiding WAR conflicts, as
there cannot be a vector store to the same memory location.

[arsenm]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

987

Doesn't this normally use a register value based dependency? Why does it need to be conservative?

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

987

This isn't handling a register dependency, it's D71934 "need to insert wait between the scalar load and vector store to the same address to avoid WAR conflict".

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1489–1490

If we don't do anything here when !Ptr, then we have lost the information that there is an outstanding scalar load, which does not seem safe to me.

Stepping back a bit, I don't think the original SLoadAddresses implementation in D71934 was safe. Surely a scalar load and a vector store can alias even if they don't have an identical Ptr value?

[piotr]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1489–1490

Agreed that my change is not correct either.

Looks D71934 was a best effort fix and did not intend to be a final one ("Proper fix should include creating the alias analysis on the machine IR that is obviously too big hummer at the moment.").

The reason I started looking at this is that the map gets corrupted if there are multiple elements with the same key and different values.

Possibly, I should be looking at places where the memop data is dropped and fix that at source.

[piotr]

Re-purposing patch to not add invariant loads to the map.

[arsenm]

Can you also add a mir test

[piotr]

Added mir test.

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1492

Might be worth asserting that Ptr is not null here, since that's what caused the problem in the first place?

[piotr]

Added the assert. The assert seems in order here - no hits in the lit tests or Vulkan CTS. There would have been hits in 194 lit tests if the assert had been placed here without the isInvariant check, which somewhat proves usefulness of the patch.

[foad]

In D101177#2753324, @piotr wrote:

Added the assert. The assert seems in order here - no hits in the lit tests or Vulkan CTS. There would have been hits in 194 lit tests if the assert had been placed here without the isInvariant check, which somewhat proves usefulness of the patch.

Thanks!

[arsenm]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1492

Not having the value set is *not* an error. This should not be an assert. This is a situation that needs to work. If we had two loads if converted into a select on the pointer, it would have been correct to strip out the IR value

[piotr]

Assert removed in a4db7025a9762c568c7bc9fdd3c64f4a60e31cfc.

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1492

OK, but just removing the assert is no good, because then we'll be back in the same situation of adding nullptr as a map key and getting non-deterministic compiles.

[piotr]

Yes, based on Matt's last comment, there is still a potential problem even though my patch significantly reduces the likelihood of it occurring.

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

if (MI.isCall() && callWaitsOnFunctionEntry(MI)) {
       // instruction.
       //
       // Two cases are handled for destination operands:
       // 1) If the destination operand was defined by a load, add the s_waitcnt
       // instruction to guarantee the right WAW order.
       // 2) If a destination operand that was used by a recent export/store ins,
       // add s_waitcnt on exp_cnt to guarantee the WAR order.
       for (const MachineMemOperand *Memop : MI.memoperands()) {
+        if (Memop->isStore()) {
           const Value *Ptr = Memop->getValue();
-        if (Memop->isStore() && SLoadAddresses.count(Ptr)) {
+          if (!Ptr) {
+            // Conservatively add the s_waitcnt in the absence of memop data.
+            addWait(Wait, LGKM_CNT, 0);
+          } else if (SLoadAddresses.count(Ptr)) {
             addWait(Wait, LGKM_CNT, 0);
-          if (PDT->dominates(MI.getParent(), SLoadAddresses.find(Ptr)->second))
+            if (PDT->dominates(MI.getParent(),
+                               SLoadAddresses.find(Ptr)->second))
               SLoadAddresses.erase(Ptr);
           }
+        }
         unsigned AS = Memop->getAddrSpace();
         if (AS != AMDGPUAS::LOCAL_ADDRESS)
           continue;
         unsigned RegNo = SQ_MAX_PGM_VGPRS + EXTRA_VGPR_LDS;
         // VM_CNT is only relevant to vgpr or LDS.
         ScoreBrackets.determineWait(
             VM_CNT, ScoreBrackets.getRegScore(RegNo, VM_CNT), Wait);
         if (Memop->isStore()) {
if (ST->hasReadVCCZBug() || !ST->partialVCCWritesUpdateVCCZ()) {
           VCCZCorrect = true;
         }
       }
     }
 
     if (TII->isSMRD(Inst)) {
       for (const MachineMemOperand *Memop : Inst.memoperands()) {
         const Value *Ptr = Memop->getValue();
+        if (Ptr)
           SLoadAddresses.insert(std::make_pair(Ptr, Inst.getParent()));
       }
       if (ST->hasReadVCCZBug()) {
         // This smem read could complete and clobber vccz at any time.
         VCCZCorrect = false;
       }
     }
 
     updateEventWaitcntAfter(Inst, &ScoreBrackets);
 
 #if 0 // TODO: implement resource type check controlled by options with ub = LB.

llvm/test/CodeGen/AMDGPU/GlobalISel/insertelement-stack-lower.ll

 ; GCN-NEXT:    v_mov_b32_e32 v9, s45
 ; GCN-NEXT:    v_mov_b32_e32 v10, s46
 ; GCN-NEXT:    v_mov_b32_e32 v11, s47
 ; GCN-NEXT:    v_mov_b32_e32 v12, s48
 ; GCN-NEXT:    v_mov_b32_e32 v13, s49
 ; GCN-NEXT:    v_mov_b32_e32 v14, s50
 ; GCN-NEXT:    v_mov_b32_e32 v15, s51
 ; GCN-NEXT:    s_load_dwordx16 s[36:51], s[10:11], 0xc0
+; GCN-NEXT:    s_waitcnt lgkmcnt(0)
 ; GCN-NEXT:    buffer_store_dword v0, off, s[0:3], 0 offset:256
 ; GCN-NEXT:    buffer_store_dword v1, off, s[0:3], 0 offset:260
 ; GCN-NEXT:    buffer_store_dword v2, off, s[0:3], 0 offset:264
 ; GCN-NEXT:    buffer_store_dword v3, off, s[0:3], 0 offset:268
 ; GCN-NEXT:    buffer_store_dword v4, off, s[0:3], 0 offset:272
 ; GCN-NEXT:    buffer_store_dword v5, off, s[0:3], 0 offset:276
 ; GCN-NEXT:    buffer_store_dword v6, off, s[0:3], 0 offset:280
 ; GCN-NEXT:    buffer_store_dword v7, off, s[0:3], 0 offset:284

 ; GCN-NEXT:    v_mov_b32_e32 v0, s24
 ; GCN-NEXT:    buffer_store_dword v0, off, s[0:3], 0 offset:432
 ; GCN-NEXT:    v_mov_b32_e32 v0, s25
 ; GCN-NEXT:    buffer_store_dword v0, off, s[0:3], 0 offset:436
 ; GCN-NEXT:    v_mov_b32_e32 v0, s26
 ; GCN-NEXT:    buffer_store_dword v0, off, s[0:3], 0 offset:440
 ; GCN-NEXT:    v_mov_b32_e32 v0, s27
 ; GCN-NEXT:    buffer_store_dword v0, off, s[0:3], 0 offset:444
-; GCN-NEXT:    s_waitcnt lgkmcnt(0)
 ; GCN-NEXT:    v_mov_b32_e32 v0, s36
 ; GCN-NEXT:    buffer_store_dword v0, off, s[0:3], 0 offset:448
 ; GCN-NEXT:    v_mov_b32_e32 v0, s37
 ; GCN-NEXT:    buffer_store_dword v0, off, s[0:3], 0 offset:452
 ; GCN-NEXT:    v_mov_b32_e32 v0, s38
 ; GCN-NEXT:    buffer_store_dword v0, off, s[0:3], 0 offset:456
 ; GCN-NEXT:    v_mov_b32_e32 v0, s39
 ; GCN-NEXT:    buffer_store_dword v0, off, s[0:3], 0 offset:460

llvm/test/CodeGen/AMDGPU/gfx-callable-argument-types.ll

 ; GFX9-NEXT:    v_writelane_b32 v40, s50, 14
 ; GFX9-NEXT:    v_writelane_b32 v40, s51, 15
 ; GFX9-NEXT:    s_waitcnt lgkmcnt(0)
 ; GFX9-NEXT:    s_load_dwordx16 s[4:19], s[20:21], 0x0
 ; GFX9-NEXT:    s_load_dwordx16 s[36:51], s[20:21], 0x40
 ; GFX9-NEXT:    s_mov_b32 s33, s32
 ; GFX9-NEXT:    v_mov_b32_e32 v0, s22
 ; GFX9-NEXT:    s_add_u32 s32, s32, 0x400
-; GFX9-NEXT:    buffer_store_dword v0, off, s[0:3], s32 offset:24
 ; GFX9-NEXT:    s_waitcnt lgkmcnt(0)
+; GFX9-NEXT:    buffer_store_dword v0, off, s[0:3], s32 offset:24
 ; GFX9-NEXT:    v_mov_b32_e32 v0, s46
 ; GFX9-NEXT:    v_mov_b32_e32 v1, s47
 ; GFX9-NEXT:    buffer_store_dword v0, off, s[0:3], s32
 ; GFX9-NEXT:    buffer_store_dword v1, off, s[0:3], s32 offset:4
 ; GFX9-NEXT:    v_mov_b32_e32 v0, s48
 ; GFX9-NEXT:    buffer_store_dword v0, off, s[0:3], s32 offset:8
 ; GFX9-NEXT:    v_mov_b32_e32 v0, s49
 ; GFX9-NEXT:    buffer_store_dword v0, off, s[0:3], s32 offset:12

 ; GFX10-NEXT:    v_writelane_b32 v40, s47, 11
 ; GFX10-NEXT:    v_writelane_b32 v40, s48, 12
 ; GFX10-NEXT:    v_writelane_b32 v40, s49, 13
 ; GFX10-NEXT:    v_writelane_b32 v40, s50, 14
 ; GFX10-NEXT:    v_writelane_b32 v40, s51, 15
 ; GFX10-NEXT:    s_clause 0x1
 ; GFX10-NEXT:    s_load_dwordx16 s[36:51], s[20:21], 0x40
 ; GFX10-NEXT:    s_load_dwordx16 s[4:19], s[20:21], 0x0
+; GFX10-NEXT:    s_waitcnt lgkmcnt(0)
 ; GFX10-NEXT:    buffer_store_dword v0, off, s[0:3], s32 offset:24
 ; GFX10-NEXT:    v_writelane_b32 v40, s30, 16
 ; GFX10-NEXT:    v_writelane_b32 v40, s31, 17
 ; GFX10-NEXT:    s_getpc_b64 s[30:31]
 ; GFX10-NEXT:    s_add_u32 s30, s30, external_void_func_v32i32_i32_inreg@rel32@lo+4
 ; GFX10-NEXT:    s_addc_u32 s31, s31, external_void_func_v32i32_i32_inreg@rel32@hi+12
-; GFX10-NEXT:    s_waitcnt lgkmcnt(0)
 ; GFX10-NEXT:    v_mov_b32_e32 v0, s46
 ; GFX10-NEXT:    v_mov_b32_e32 v1, s47
 ; GFX10-NEXT:    v_mov_b32_e32 v2, s48
 ; GFX10-NEXT:    v_mov_b32_e32 v3, s49
 ; GFX10-NEXT:    s_mov_b32 s20, s36
 ; GFX10-NEXT:    s_mov_b32 s21, s37
 ; GFX10-NEXT:    s_mov_b32 s22, s38
 ; GFX10-NEXT:    s_mov_b32 s23, s39

llvm/test/CodeGen/AMDGPU/load-local-redundant-copies.ll

 ; CHECK-NEXT:    ds_read_b32 v4, v2
 ; CHECK-NEXT:    ds_read_b32 v3, v3
 ; CHECK-NEXT:    ds_read_b32 v2, v6
 ; CHECK-NEXT:    ds_read_b32 v9, v7
 ; CHECK-NEXT:    ds_read_b32 v8, v8
 ; CHECK-NEXT:    ds_read_b32 v7, v10
 ; CHECK-NEXT:    ds_read_b32 v6, v1
 ; CHECK-NEXT:    ds_read_b32 v5, v5
-; CHECK-NEXT:    s_waitcnt lgkmcnt(1)
-; CHECK-NEXT:    tbuffer_store_format_xyzw v[6:9], v0, s[0:3], s4 format:[BUF_DATA_FORMAT_32_32_32,BUF_NUM_FORMAT_UINT] idxen glc slc
 ; CHECK-NEXT:    s_waitcnt lgkmcnt(0)
+; CHECK-NEXT:    tbuffer_store_format_xyzw v[6:9], v0, s[0:3], s4 format:[BUF_DATA_FORMAT_32_32_32,BUF_NUM_FORMAT_UINT] idxen glc slc
 ; CHECK-NEXT:    tbuffer_store_format_xyzw v[2:5], v0, s[0:3], s4 format:[BUF_DATA_FORMAT_32_32_32,BUF_NUM_FORMAT_UINT] idxen offset:16 glc slc
 ; CHECK-NEXT:    s_endpgm
   %load = load <8 x float>, <8 x float> addrspace(3)* %arg4, align 4
   %vec1 = shufflevector <8 x float> %load, <8 x float> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
   call void @llvm.amdgcn.struct.tbuffer.store.v4f32(<4 x float> %vec1, <4 x i32> %arg1, i32 %arg2, i32 0, i32 %arg3, i32 immarg 77, i32 immarg 3)
   %vec2 = shufflevector <8 x float> %load, <8 x float> undef, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
   call void @llvm.amdgcn.struct.tbuffer.store.v4f32(<4 x float> %vec2, <4 x i32> %arg1, i32 %arg2, i32 16, i32 %arg3, i32 immarg 77, i32 immarg 3)
   ret void

 ; CHECK-NEXT:    ds_read_b32 v3, v1
 ; CHECK-NEXT:    ds_read_b32 v5, v4
 ; CHECK-NEXT:    ds_read_b32 v4, v7
 ; CHECK-NEXT:    ds_read_b32 v1, v8
 ; CHECK-NEXT:    ds_read_b32 v6, v6
 ; CHECK-NEXT:    ds_read_b32 v0, v0
 ; CHECK-NEXT:    v_add_i32_e32 v7, vcc, 4, v2
 ; CHECK-NEXT:    v_add_i32_e32 v8, vcc, 20, v2
-; CHECK-NEXT:    s_waitcnt lgkmcnt(1)
-; CHECK-NEXT:    tbuffer_store_format_xyzw v[3:6], v9, s[4:7], s1 format:[BUF_DATA_FORMAT_32_32_32,BUF_NUM_FORMAT_UINT] idxen offset:264 glc slc
 ; CHECK-NEXT:    s_waitcnt lgkmcnt(0)
+; CHECK-NEXT:    tbuffer_store_format_xyzw v[3:6], v9, s[4:7], s1 format:[BUF_DATA_FORMAT_32_32_32,BUF_NUM_FORMAT_UINT] idxen offset:264 glc slc
 ; CHECK-NEXT:    tbuffer_store_format_xy v[0:1], v9, s[4:7], s1 format:[BUF_DATA_FORMAT_INVALID,BUF_NUM_FORMAT_UINT] idxen offset:280 glc slc
 ; CHECK-NEXT:    s_waitcnt expcnt(0)
 ; CHECK-NEXT:    ds_read_b32 v0, v2
 ; CHECK-NEXT:    ds_read_b32 v2, v12
 ; CHECK-NEXT:    ds_read_b32 v1, v7
 ; CHECK-NEXT:    ds_read_b32 v5, v8
 ; CHECK-NEXT:    ds_read_b32 v3, v11
 ; CHECK-NEXT:    ds_read_b32 v4, v10
 ; CHECK-NEXT:    s_waitcnt lgkmcnt(5)
 ; CHECK-NEXT:    exp mrt0 off, off, off, off
-; CHECK-NEXT:    s_waitcnt lgkmcnt(1)
-; CHECK-NEXT:    tbuffer_store_format_xyzw v[0:3], v9, s[4:7], s1 format:[BUF_DATA_FORMAT_32_32_32,BUF_NUM_FORMAT_UINT] idxen offset:240 glc slc
 ; CHECK-NEXT:    s_waitcnt lgkmcnt(0)
+; CHECK-NEXT:    tbuffer_store_format_xyzw v[0:3], v9, s[4:7], s1 format:[BUF_DATA_FORMAT_32_32_32,BUF_NUM_FORMAT_UINT] idxen offset:240 glc slc
 ; CHECK-NEXT:    tbuffer_store_format_xy v[4:5], v9, s[4:7], s1 format:[BUF_DATA_FORMAT_INVALID,BUF_NUM_FORMAT_UINT] idxen offset:256 glc slc
 ; CHECK-NEXT:    s_endpgm
   %load1 = load <6 x float>, <6 x float> addrspace(3)* %arg5, align 4
   %vec11 = shufflevector <6 x float> %load1, <6 x float> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
   call void @llvm.amdgcn.struct.tbuffer.store.v4f32(<4 x float> %vec11, <4 x i32> %arg3, i32 %arg1, i32 264, i32 %arg2, i32 immarg 77, i32 immarg 3)
   %vec12 = shufflevector <6 x float> %load1, <6 x float> undef, <2 x i32> <i32 4, i32 5>
   call void @llvm.amdgcn.struct.tbuffer.store.v2f32(<2 x float> %vec12, <4 x i32> %arg3, i32 %arg1, i32 280, i32 %arg2, i32 immarg 64, i32 immarg 3)
 

llvm/test/CodeGen/AMDGPU/partial-sgpr-to-vgpr-spills.ll

 ; GCN-NEXT:    v_writelane_b32 v31, s17, 61
 ; GCN-NEXT:    v_writelane_b32 v31, s18, 62
 ; GCN-NEXT:    v_writelane_b32 v31, s19, 63
 ; GCN-NEXT:    ;;#ASMSTART
 ; GCN-NEXT:    ; def s[2:3]
 ; GCN-NEXT:    ;;#ASMEND
 ; GCN-NEXT:    s_mov_b64 s[4:5], exec
 ; GCN-NEXT:    s_mov_b64 exec, 3
+; GCN-NEXT:    s_waitcnt lgkmcnt(0)
 ; GCN-NEXT:    buffer_store_dword v0, off, s[52:55], 0
 ; GCN-NEXT:    v_writelane_b32 v0, s2, 0
 ; GCN-NEXT:    v_writelane_b32 v0, s3, 1
 ; GCN-NEXT:    buffer_store_dword v0, off, s[52:55], 0 offset:4 ; 4-byte Folded Spill
 ; GCN-NEXT:    buffer_load_dword v0, off, s[52:55], 0
 ; GCN-NEXT:    s_waitcnt vmcnt(0)
 ; GCN-NEXT:    s_mov_b64 exec, s[4:5]
 ; GCN-NEXT:    s_mov_b32 s1, 0
-; GCN-NEXT:    s_waitcnt lgkmcnt(0)
 ; GCN-NEXT:    s_cmp_lg_u32 s0, s1
 ; GCN-NEXT:    s_cbranch_scc1 BB2_2
 ; GCN-NEXT:  ; %bb.1: ; %bb0
 ; GCN-NEXT:    v_readlane_b32 s36, v31, 32
 ; GCN-NEXT:    v_readlane_b32 s37, v31, 33
 ; GCN-NEXT:    v_readlane_b32 s38, v31, 34
 ; GCN-NEXT:    v_readlane_b32 s39, v31, 35
 ; GCN-NEXT:    v_readlane_b32 s40, v31, 36

 ; GCN-NEXT:    v_writelane_b32 v31, s17, 61
 ; GCN-NEXT:    v_writelane_b32 v31, s18, 62
 ; GCN-NEXT:    v_writelane_b32 v31, s19, 63
 ; GCN-NEXT:    ;;#ASMSTART
 ; GCN-NEXT:    ; def s[2:3]
 ; GCN-NEXT:    ;;#ASMEND
 ; GCN-NEXT:    s_mov_b64 s[4:5], exec
 ; GCN-NEXT:    s_mov_b64 exec, 3
+; GCN-NEXT:    s_waitcnt lgkmcnt(0)
 ; GCN-NEXT:    buffer_store_dword v0, off, s[52:55], 0
 ; GCN-NEXT:    v_writelane_b32 v0, s2, 0
 ; GCN-NEXT:    v_writelane_b32 v0, s3, 1
 ; GCN-NEXT:    buffer_store_dword v0, off, s[52:55], 0 offset:4 ; 4-byte Folded Spill
 ; GCN-NEXT:    buffer_load_dword v0, off, s[52:55], 0
 ; GCN-NEXT:    s_waitcnt vmcnt(0)
 ; GCN-NEXT:    s_mov_b64 exec, s[4:5]
 ; GCN-NEXT:    s_mov_b32 s1, 0
-; GCN-NEXT:    s_waitcnt lgkmcnt(0)
 ; GCN-NEXT:    s_cmp_lg_u32 s0, s1
 ; GCN-NEXT:    s_cbranch_scc1 BB3_2
 ; GCN-NEXT:  ; %bb.1: ; %bb0
 ; GCN-NEXT:    v_readlane_b32 s36, v31, 32
 ; GCN-NEXT:    v_readlane_b32 s37, v31, 33
 ; GCN-NEXT:    v_readlane_b32 s38, v31, 34
 ; GCN-NEXT:    v_readlane_b32 s39, v31, 35
 ; GCN-NEXT:    v_readlane_b32 s40, v31, 36

llvm/test/CodeGen/AMDGPU/waitcnt-vscnt.mir

 # RUN: llc -march=amdgcn -mcpu=gfx1010 -verify-machineinstrs -run-pass si-insert-waitcnts -o - %s | FileCheck -check-prefix=GFX10 %s
 
 # GFX10-LABEL: waitcnt-vscnt
 # GFX10: GLOBAL_ATOMIC_ADD_RTN
 # GFX10-NEXT: S_WAITCNT 49279
 ---
 name: waitcnt-vscnt
 machineFunctionInfo:
   isEntryFunction: true
 body: |
   bb.0:
     liveins: $sgpr0_sgpr1
     $sgpr4 = S_LOAD_DWORD_IMM $sgpr0_sgpr1, 4, 0 :: (dereferenceable invariant load 4 from `i32 addrspace(4)* undef`)
     S_WAITCNT_VSCNT undef $sgpr_null, 0
-    $vgpr0 = GLOBAL_ATOMIC_ADD_RTN $vgpr0_vgpr1, $vgpr2, 0, 1, implicit $exec :: (load store syncscope("agent") seq_cst 4, addrspace 1)
+    $vgpr0 = GLOBAL_ATOMIC_ADD_RTN $vgpr0_vgpr1, $vgpr2, 0, 1, implicit $exec :: (load store syncscope("agent") seq_cst 4 on `i32 addrspace(1)* undef`)
     S_CMP_LG_U32 killed $sgpr4, 0, implicit-def $scc
 ...