[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

1096

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

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1096

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

1526–1532

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

1526–1532

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

1529

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

1529

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

1529

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)) {
         const Value *Ptr = Memop->getValue();
         if (Memop->isStore() && SLoadAddresses.count(Ptr)) {
           addWait(Wait, LGKM_CNT, 0);
           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()) {
           ScoreBrackets.determineWait(
               EXP_CNT, ScoreBrackets.getRegScore(RegNo, EXP_CNT), Wait);
         }
if (ST->hasReadVCCZBug() || !ST->partialVCCWritesUpdateVCCZ()) {
           // Writes to vcc will fix any incorrect value in vccz.
           VCCZCorrect = true;
         }
       }
     }
 
     if (TII->isSMRD(Inst)) {
       for (const MachineMemOperand *Memop : Inst.memoperands()) {
+        // No need to handle invariant loads when avoiding WAR conflicts, as
+        // there cannot be a vector store to the same memory location.
+        if (!Memop->isInvariant()) {
           const Value *Ptr = Memop->getValue();
           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);
 

llvm/test/CodeGen/AMDGPU/mul24-pass-ordering.ll

; CHECK-NOT: mul i32
   %mul2 = mul i32 %b2, %s
   store volatile i32 %mul2, i32 addrspace(1)* undef
   ret void
 }
 
 define void @slsr1_1(i32 %b.arg, i32 %s.arg) #0 {
 ; GFX9-LABEL: slsr1_1:
 ; GFX9:       ; %bb.0:
-; GFX9-NEXT:  s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; GFX9-NEXT:  s_or_saveexec_b64 s[4:5], -1
-; GFX9-NEXT:  buffer_store_dword v43, off, s[0:3], s32 offset:12 ; 4-byte Folded Spill
-; GFX9-NEXT:  s_mov_b64 exec, s[4:5]
-; GFX9-NEXT:  v_writelane_b32 v43, s33, 4
-; GFX9-NEXT:  s_mov_b32 s33, s32
-; GFX9-NEXT:  s_add_u32 s32, s32, 0x800
-; GFX9-NEXT:  v_writelane_b32 v43, s34, 0
-; GFX9-NEXT:  s_getpc_b64 s[4:5]
-; GFX9-NEXT:  s_add_u32 s4, s4, foo@gotpcrel32@lo+4
-; GFX9-NEXT:  s_addc_u32 s5, s5, foo@gotpcrel32@hi+12
-; GFX9-NEXT:  v_writelane_b32 v43, s35, 1
-; GFX9-NEXT:  s_load_dwordx2 s[34:35], s[4:5], 0x0
-; GFX9-NEXT:  s_waitcnt lgkmcnt(0)
-; GFX9-NEXT:  buffer_store_dword v40, off, s[0:3], s33 offset:8 ; 4-byte Folded Spill
-; GFX9-NEXT:  buffer_store_dword v41, off, s[0:3], s33 offset:4 ; 4-byte Folded Spill
-; GFX9-NEXT:  buffer_store_dword v42, off, s[0:3], s33 ; 4-byte Folded Spill
-; GFX9-NEXT:  v_mov_b32_e32 v40, v1
-; GFX9-NEXT:  v_mov_b32_e32 v41, v0
-; GFX9-NEXT:  v_writelane_b32 v43, s30, 2
-; GFX9-NEXT:  v_mul_u32_u24_e32 v0, v41, v40
-; GFX9-NEXT:  v_writelane_b32 v43, s31, 3
-; GFX9-NEXT:  v_and_b32_e32 v42, 0xffffff, v40
-; GFX9-NEXT:  s_swappc_b64 s[30:31], s[34:35]
-; GFX9-NEXT:  v_mad_u32_u24 v40, v41, v40, v42
-; GFX9-NEXT:  v_mov_b32_e32 v0, v40
-; GFX9-NEXT:  s_swappc_b64 s[30:31], s[34:35]
-; GFX9-NEXT:  v_add_u32_e32 v0, v40, v42
-; GFX9-NEXT:  s_swappc_b64 s[30:31], s[34:35]
-; GFX9-NEXT:  buffer_load_dword v42, off, s[0:3], s33 ; 4-byte Folded Reload
-; GFX9-NEXT:  buffer_load_dword v41, off, s[0:3], s33 offset:4 ; 4-byte Folded Reload
-; GFX9-NEXT:  buffer_load_dword v40, off, s[0:3], s33 offset:8 ; 4-byte Folded Reload
-; GFX9-NEXT:  v_readlane_b32 s4, v43, 2
-; GFX9-NEXT:  v_readlane_b32 s5, v43, 3
-; GFX9-NEXT:  v_readlane_b32 s35, v43, 1
-; GFX9-NEXT:  v_readlane_b32 s34, v43, 0
-; GFX9-NEXT:  s_sub_u32 s32, s32, 0x800
-; GFX9-NEXT:  v_readlane_b32 s33, v43, 4
-; GFX9-NEXT:  s_or_saveexec_b64 s[6:7], -1
-; GFX9-NEXT:  buffer_load_dword v43, off, s[0:3], s32 offset:12 ; 4-byte Folded Reload
-; GFX9-NEXT:  s_mov_b64 exec, s[6:7]
-; GFX9-NEXT:  s_waitcnt vmcnt(0)
-; GFX9-NEXT:  s_setpc_b64 s[4:5]
+; GFX9-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GFX9-NEXT:    s_or_saveexec_b64 s[4:5], -1
+; GFX9-NEXT:    buffer_store_dword v43, off, s[0:3], s32 offset:12 ; 4-byte Folded Spill
+; GFX9-NEXT:    s_mov_b64 exec, s[4:5]
+; GFX9-NEXT:    v_writelane_b32 v43, s33, 4
+; GFX9-NEXT:    s_mov_b32 s33, s32
+; GFX9-NEXT:    s_add_u32 s32, s32, 0x800
+; GFX9-NEXT:    v_writelane_b32 v43, s34, 0
+; GFX9-NEXT:    s_getpc_b64 s[4:5]
+; GFX9-NEXT:    s_add_u32 s4, s4, foo@gotpcrel32@lo+4
+; GFX9-NEXT:    s_addc_u32 s5, s5, foo@gotpcrel32@hi+12
+; GFX9-NEXT:    v_writelane_b32 v43, s35, 1
+; GFX9-NEXT:    s_load_dwordx2 s[34:35], s[4:5], 0x0
+; GFX9-NEXT:    buffer_store_dword v40, off, s[0:3], s33 offset:8 ; 4-byte Folded Spill
+; GFX9-NEXT:    buffer_store_dword v41, off, s[0:3], s33 offset:4 ; 4-byte Folded Spill
+; GFX9-NEXT:    buffer_store_dword v42, off, s[0:3], s33 ; 4-byte Folded Spill
+; GFX9-NEXT:    v_mov_b32_e32 v40, v1
+; GFX9-NEXT:    v_mov_b32_e32 v41, v0
+; GFX9-NEXT:    v_writelane_b32 v43, s30, 2
+; GFX9-NEXT:    v_mul_u32_u24_e32 v0, v41, v40
+; GFX9-NEXT:    v_writelane_b32 v43, s31, 3
+; GFX9-NEXT:    v_and_b32_e32 v42, 0xffffff, v40
+; GFX9-NEXT:    s_waitcnt lgkmcnt(0)
+; GFX9-NEXT:    s_swappc_b64 s[30:31], s[34:35]
+; GFX9-NEXT:    v_mad_u32_u24 v40, v41, v40, v42
+; GFX9-NEXT:    v_mov_b32_e32 v0, v40
+; GFX9-NEXT:    s_swappc_b64 s[30:31], s[34:35]
+; GFX9-NEXT:    v_add_u32_e32 v0, v40, v42
+; GFX9-NEXT:    s_swappc_b64 s[30:31], s[34:35]
+; GFX9-NEXT:    buffer_load_dword v42, off, s[0:3], s33 ; 4-byte Folded Reload
+; GFX9-NEXT:    buffer_load_dword v41, off, s[0:3], s33 offset:4 ; 4-byte Folded Reload
+; GFX9-NEXT:    buffer_load_dword v40, off, s[0:3], s33 offset:8 ; 4-byte Folded Reload
+; GFX9-NEXT:    v_readlane_b32 s4, v43, 2
+; GFX9-NEXT:    v_readlane_b32 s5, v43, 3
+; GFX9-NEXT:    v_readlane_b32 s35, v43, 1
+; GFX9-NEXT:    v_readlane_b32 s34, v43, 0
+; GFX9-NEXT:    s_sub_u32 s32, s32, 0x800
+; GFX9-NEXT:    v_readlane_b32 s33, v43, 4
+; GFX9-NEXT:    s_or_saveexec_b64 s[6:7], -1
+; GFX9-NEXT:    buffer_load_dword v43, off, s[0:3], s32 offset:12 ; 4-byte Folded Reload
+; GFX9-NEXT:    s_mov_b64 exec, s[6:7]
+; GFX9-NEXT:    s_waitcnt vmcnt(0)
+; GFX9-NEXT:    s_setpc_b64 s[4:5]
   %b = and i32 %b.arg, 16777215
   %s = and i32 %s.arg, 16777215
 
 ; CHECK-LABEL: @slsr1(
   ; foo(b * s);
   %mul0 = mul i32 %b, %s
 ; CHECK: mul i32
 ; CHECK-NOT: mul i32

llvm/test/CodeGen/AMDGPU/stack-realign.ll

 ; GCN: s_and_b32 s33, [[SCRATCH_REG]], 0xffff0000
 ; GCN: s_mov_b32 s34, s32
 ; GCN: v_mov_b32_e32 v32, 0
 ; GCN: buffer_store_dword v32, off, s[0:3], s33 offset:1024
 ; GCN-NEXT: s_waitcnt vmcnt(0)
 ; GCN-NEXT: buffer_load_dword v{{[0-9]+}}, off, s[0:3], s34
 ; GCN-DAG: s_add_u32 s32, s32, 0x30000
 ; GCN: buffer_store_dword v{{[0-9]+}}, off, s[0:3], s32
-; GCN-NEXT: s_swappc_b64 s[30:31], s[4:5]
+; GCN: s_swappc_b64 s[30:31], s[4:5]
 
 ; GCN: s_sub_u32 s32, s32, 0x30000
 ; GCN-NEXT: v_readlane_b32 s33, [[VGPR_REG]], 2
 ; GCN-NEXT: v_readlane_b32 s34, [[VGPR_REG]], 3
 ; GCN-NEXT: s_or_saveexec_b64 s[6:7], -1
 ; GCN-NEXT: buffer_load_dword [[VGPR_REG]], off, s[0:3], s32 offset:1028 ; 4-byte Folded Reload
 ; GCN-NEXT: s_mov_b64 exec, s[6:7]
   %temp = alloca i32, align 1024, addrspace(5)

llvm/test/CodeGen/AMDGPU/waitcnt-no-redundant.mir

body: |
   bb.1:
     S_WAITCNT 3952
     FLAT_ATOMIC_CMPSWAP undef renamable $vgpr0_vgpr1, renamable $vgpr0_vgpr1, 0, 0, implicit $exec, implicit $flat_scr
     S_WAITCNT 3952
     BUFFER_WBINVL1 implicit $exec
     S_BRANCH %bb.1
 
 ...
+
+# Check that the waitcnt pass does *not* insert a waitcnt inst after S_BUFFER_LOAD.
+# WAR hazard does not apply here, because S_BUFFER_LOAD accesses invariant memory.
+...
+# CHECK-LABEL: name: waitcnt-no-war-wait
+# CHECK: S_WAITCNT 0
+# CHECK-NEXT: S_BUFFER_LOAD_DWORD_IMM
+# CHECK-NEXT: TBUFFER_STORE_FORMAT_X_OFFEN_exact
+name: waitcnt-no-war-wait
+body: |
+  bb.0:
+    renamable $sgpr8 = S_BUFFER_LOAD_DWORD_IMM renamable $sgpr0_sgpr1_sgpr2_sgpr3, 276, 0 :: (dereferenceable invariant load 4)
+    TBUFFER_STORE_FORMAT_X_OFFEN_exact killed renamable $vgpr0, renamable $vgpr15, renamable $sgpr4_sgpr5_sgpr6_sgpr7, renamable $sgpr9, 0, 116, 1, 0, 0, implicit $exec :: (dereferenceable store 4 into custom "BufferResource", align 1, addrspace 4)
+
+...