[AMDGPU][SIInsertWaitcnts] Wait on all LDS DMA operations when no aliasing store is found #170660

Pierre-vh · 2025-12-04T13:52:16Z

Previously, we would miss inserting a wait if the ds_read had AA info, but it didn't match
any LDS DMA op, for example if we didn't track the LDS DMA op it aliases with because it exceeded the tracking limit.

…asing store is found Previously, we would miss inserting a wait if the ds_read had AA info, but it didn't match any LDS DMA op.

Pierre-vh · 2025-12-04T13:52:36Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

llvmbot · 2025-12-04T14:01:30Z

@llvm/pr-subscribers-backend-amdgpu

Author: Pierre van Houtryve (Pierre-vh)

Changes

Previously, we would miss inserting a wait if the ds_read had AA info, but it didn't match
any LDS DMA op.

Full diff: https://github.com/llvm/llvm-project/pull/170660.diff

3 Files Affected:

(modified) llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp (+13-5)
(modified) llvm/test/CodeGen/AMDGPU/lds-dma-waits.ll (+4-1)
(modified) llvm/test/CodeGen/AMDGPU/waitcnt-unscoped.ll (+1)

diff --git a/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp b/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
index 79c3394b2df50..69fae98d4a13f 100644
--- a/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
@@ -2007,18 +2007,26 @@ bool SIInsertWaitcnts::generateWaitcntInstBefore(MachineInstr &MI,
 
         // LOAD_CNT is only relevant to vgpr or LDS.
         unsigned RegNo = FIRST_LDS_VGPR;
+        bool FoundAliasingStore = false;
         if (Ptr && Memop->getAAInfo()) {
           const auto &LDSDMAStores = ScoreBrackets.getLDSDMAStores();
           for (unsigned I = 0, E = LDSDMAStores.size(); I != E; ++I) {
-            if (MI.mayAlias(AA, *LDSDMAStores[I], true))
+            if (MI.mayAlias(AA, *LDSDMAStores[I], true)) {
+              FoundAliasingStore = true;
               ScoreBrackets.determineWait(LOAD_CNT, RegNo + I + 1, Wait);
+            }
           }
-        } else {
-          ScoreBrackets.determineWait(LOAD_CNT, RegNo, Wait);
         }
-        if (Memop->isStore()) {
+
+        // TODO?: Is it possible to have cases where we'd alias with both a
+        // store tracked in LDSDMAStores, and one that isn't ? If so, the
+        // current system would only wait on the tracked store, and not the
+        // "generic" entry.
+        if (!FoundAliasingStore)
+          ScoreBrackets.determineWait(LOAD_CNT, RegNo, Wait);
+
+        if (Memop->isStore())
           ScoreBrackets.determineWait(EXP_CNT, RegNo, Wait);
-        }
       }
 
       // Loop over use and def operands.
diff --git a/llvm/test/CodeGen/AMDGPU/lds-dma-waits.ll b/llvm/test/CodeGen/AMDGPU/lds-dma-waits.ll
index 37ba1f42413c9..1cf0fa2768de6 100644
--- a/llvm/test/CodeGen/AMDGPU/lds-dma-waits.ll
+++ b/llvm/test/CodeGen/AMDGPU/lds-dma-waits.ll
@@ -223,6 +223,7 @@ define amdgpu_kernel void @buffer_load_lds_dword_10_arrays(<4 x i32> %rsrc, i32
 ; GFX9-NEXT:    s_waitcnt vmcnt(2)
 ; GFX9-NEXT:    ds_read_b32 v7, v9 offset:1792
 ; GFX9-NEXT:    ; wave barrier
+; GFX9-NEXT:    s_waitcnt vmcnt(0)
 ; GFX9-NEXT:    ds_read_b32 v8, v9 offset:2048
 ; GFX9-NEXT:    ; wave barrier
 ; GFX9-NEXT:    ds_read_b32 v9, v9 offset:2304
@@ -288,6 +289,7 @@ define amdgpu_kernel void @buffer_load_lds_dword_10_arrays(<4 x i32> %rsrc, i32
 ; GFX10-NEXT:    s_waitcnt vmcnt(2)
 ; GFX10-NEXT:    ds_read_b32 v7, v9 offset:1792
 ; GFX10-NEXT:    ; wave barrier
+; GFX10-NEXT:    s_waitcnt vmcnt(0)
 ; GFX10-NEXT:    ds_read_b32 v8, v9 offset:2048
 ; GFX10-NEXT:    ; wave barrier
 ; GFX10-NEXT:    ds_read_b32 v9, v9 offset:2304
@@ -374,7 +376,7 @@ define amdgpu_kernel void @global_load_lds_no_alias_ds_read(ptr addrspace(1) noc
 ; GFX9-NEXT:    v_mov_b32_e32 v0, s0
 ; GFX9-NEXT:    s_lshl_b32 s0, s3, 2
 ; GFX9-NEXT:    v_mov_b32_e32 v1, s0
-; GFX9-NEXT:    s_waitcnt vmcnt(1)
+; GFX9-NEXT:    s_waitcnt vmcnt(0)
 ; GFX9-NEXT:    ds_read_b32 v0, v0 offset:512
 ; GFX9-NEXT:    s_waitcnt vmcnt(0)
 ; GFX9-NEXT:    ds_read_b32 v1, v1 offset:768
@@ -397,6 +399,7 @@ define amdgpu_kernel void @global_load_lds_no_alias_ds_read(ptr addrspace(1) noc
 ; GFX10-NEXT:    v_mov_b32_e32 v0, s0
 ; GFX10-NEXT:    s_lshl_b32 s0, s3, 2
 ; GFX10-NEXT:    v_mov_b32_e32 v1, s0
+; GFX10-NEXT:    s_waitcnt vmcnt(0)
 ; GFX10-NEXT:    ds_read_b32 v0, v0 offset:512
 ; GFX10-NEXT:    s_waitcnt vmcnt(0) lgkmcnt(15)
 ; GFX10-NEXT:    ds_read_b32 v1, v1 offset:768
diff --git a/llvm/test/CodeGen/AMDGPU/waitcnt-unscoped.ll b/llvm/test/CodeGen/AMDGPU/waitcnt-unscoped.ll
index a00aca34252b1..7f975549fc55e 100644
--- a/llvm/test/CodeGen/AMDGPU/waitcnt-unscoped.ll
+++ b/llvm/test/CodeGen/AMDGPU/waitcnt-unscoped.ll
@@ -23,6 +23,7 @@ define amdgpu_kernel void @test_waitcnt(ptr addrspace(1) %global_buffer, ptr add
 ; CHECK-NEXT:    ; sched_barrier mask(0x00000000)
 ; CHECK-NEXT:    v_mov_b32_e32 v1, s2
 ; CHECK-NEXT:    v_mov_b32_e32 v2, s3
+; CHECK-NEXT:    s_waitcnt vmcnt(1)
 ; CHECK-NEXT:    ds_write_b32 v1, v3
 ; CHECK-NEXT:    ds_write_b32 v2, v3
 ; CHECK-NEXT:    ; sched_barrier mask(0x00000000)

jayfoad · 2025-12-04T14:27:51Z

llvm/test/CodeGen/AMDGPU/waitcnt-unscoped.ll

 ; CHECK-NEXT:    ; sched_barrier mask(0x00000000)
 ; CHECK-NEXT:    v_mov_b32_e32 v1, s2
 ; CHECK-NEXT:    v_mov_b32_e32 v2, s3
+; CHECK-NEXT:    s_waitcnt vmcnt(1)


As an example, could you explain why this wait is required? If I understand the metadata correctly then we can assume that the "Global to LDS load" does not alias with either of the DS_WRITEs.

It's too conservative, I'll look into fixing it
I think we should let LDSDMAStores grow infinitely so we can always check the aliasing stores, and then if the ID is out of bounds we just use the default one.
It'd fix the TODO I added as well I believe.

jayfoad · 2025-12-04T15:35:17Z

llvm/test/CodeGen/AMDGPU/lds-dma-waits.ll

 ; GFX9-NEXT:    s_waitcnt vmcnt(2)
 ; GFX9-NEXT:    ds_read_b32 v7, v9 offset:1792
 ; GFX9-NEXT:    ; wave barrier
+; GFX9-NEXT:    s_waitcnt vmcnt(0)


Is this wait needed? Or is the test deliberately using more slots than SIInsertWaitcnts can track, so we have to be conservative?

Yes that's precisely what I was trying to fix. The wait is required here because we ran out of tracking slots.
#162077 increases the number of tracking slots so this will be improved

arsenm · 2025-12-04T21:31:30Z

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

          for (unsigned I = 0, E = LDSDMAStores.size(); I != E; ++I) {
-            if (MI.mayAlias(AA, *LDSDMAStores[I], true))
+            if (MI.mayAlias(AA, *LDSDMAStores[I], true)) {
+              if ((I + 1) >= NUM_LDS_VGPRS) {


Can you move this up to avoid the AA check if it the limit

We still need to do the AA check if we hit the limit. If we don't alias with any LDSDMAStore, even those above the limit, we don't need to wait

rampitec

LGTM, though commit description could talk about exceeding tracking slots limit.

jayfoad · 2025-12-08T10:11:39Z

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

+      if (Slot < NUM_LDS_VGPRS)
+        setRegScore(FIRST_LDS_VGPR + Slot, T, CurrScore);
      if (Slot)
        setRegScore(FIRST_LDS_VGPR, T, CurrScore);


Just curious: can Slot ever be 0 when we exit the loop above? It's not clear to me if these "if"s handle that case correctly, if it can ever happen.

I think so. Worst case we'll call setRegScore twice. This will be fixed in #162077

…asing store is found (llvm#170660) Previously, we would miss inserting a wait if the ds_read had AA info, but it didn't match any LDS DMA op, for example if we didn't track the LDS DMA op it aliases with because it exceeded the tracking limit.

[AMDGPU][SIInsertWaitcnts] Wait on all LDS DMA operations when no ali…

427d6c4

…asing store is found Previously, we would miss inserting a wait if the ds_read had AA info, but it didn't match any LDS DMA op.

Pierre-vh requested review from arsenm, jayfoad and ssahasra December 4, 2025 13:52

Pierre-vh mentioned this pull request Dec 4, 2025

[AMDGPU][SIInsertWaitCnts] Use RegUnits-based tracking #162077

Merged

Pierre-vh marked this pull request as ready for review December 4, 2025 14:00

llvmbot added the backend:AMDGPU label Dec 4, 2025

jayfoad reviewed Dec 4, 2025

View reviewed changes

Update fix

0551da5

Pierre-vh requested a review from jayfoad December 4, 2025 14:53

jayfoad reviewed Dec 4, 2025

View reviewed changes

arsenm reviewed Dec 4, 2025

View reviewed changes

Pierre-vh requested review from arsenm, jayfoad, rampitec and shiltian December 5, 2025 09:17

rampitec approved these changes Dec 5, 2025

View reviewed changes

ssahasra approved these changes Dec 8, 2025

View reviewed changes

Pierre-vh merged commit 8aa82ef into main Dec 8, 2025
10 checks passed

Pierre-vh deleted the users/pierre-vh/ldsdma-wait-if-no-aliass branch December 8, 2025 10:02

jayfoad reviewed Dec 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AMDGPU][SIInsertWaitcnts] Wait on all LDS DMA operations when no aliasing store is found #170660

[AMDGPU][SIInsertWaitcnts] Wait on all LDS DMA operations when no aliasing store is found #170660

Uh oh!

Pierre-vh commented Dec 4, 2025 •

edited

Loading

Uh oh!

Pierre-vh commented Dec 4, 2025

Uh oh!

llvmbot commented Dec 4, 2025

Uh oh!

jayfoad Dec 4, 2025

Uh oh!

Pierre-vh Dec 4, 2025

Uh oh!

jayfoad Dec 4, 2025

Uh oh!

Pierre-vh Dec 4, 2025

Uh oh!

arsenm Dec 4, 2025

Uh oh!

Pierre-vh Dec 5, 2025

Uh oh!

rampitec left a comment

Uh oh!

Uh oh!

jayfoad Dec 8, 2025

Uh oh!

Pierre-vh Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[AMDGPU][SIInsertWaitcnts] Wait on all LDS DMA operations when no aliasing store is found #170660

[AMDGPU][SIInsertWaitcnts] Wait on all LDS DMA operations when no aliasing store is found #170660

Uh oh!

Conversation

Pierre-vh commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pierre-vh commented Dec 4, 2025

Uh oh!

llvmbot commented Dec 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rampitec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Pierre-vh commented Dec 4, 2025 •

edited

Loading