[AMDGPU] Remove scope check in SIInsertWaitcnts::generateWaitcntInstBefore #157821
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-amdgpu Author: choikwa (choikwa) ChangesThis change was motivated by CK where many VMCNT(0)'s were generated due to instructions lacking !alias.scope metadata. The two causes of this were:
However, it turns out that IPSCCP losing the scope information was largely ineffectual as ScopedNoAliasAA was able to handle asymmetric condition, where one MemLoc was missing scope, and still return NoAlias result. AMDGPU however was checking for existence of scope in SIInsertWaitcnts and conservatively treating it as aliasing all and inserted VMCNT(0) before DS_READs, forcing it to wait for all previous LDS DMA instructions. Since we know that ScopedNoAliasAA can handle asymmetry, we should also allow AA query to determine if two MIs may alias. Passed PSDB. Previous attempt to address the issue in IPSCCP, likely stalled: #154522 Full diff: https://github.com/llvm/llvm-project/pull/157821.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp b/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
index e3a2efdd3856f..68e7bdc2b5ca8 100644
--- a/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
@@ -1943,12 +1943,10 @@ bool SIInsertWaitcnts::generateWaitcntInstBefore(MachineInstr &MI,
// LOAD_CNT is only relevant to vgpr or LDS.
unsigned RegNo = FIRST_LDS_VGPR;
// Only objects with alias scope info were added to LDSDMAScopes array.
- // In the absense of the scope info we will not be able to disambiguate
- // aliasing here. There is no need to try searching for a corresponding
- // store slot. This is conservatively correct because in that case we
- // will produce a wait using the first (general) LDS DMA wait slot which
- // will wait on all of them anyway.
- if (Ptr && Memop->getAAInfo() && Memop->getAAInfo().Scope) {
+ // AliasAnalysis query can determine aliasing even if Memop's Scope is
+ // missing. ScopedNoAlias allows for alias query on MemLoc without a
+ // scope.
+ 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))
diff --git a/llvm/test/CodeGen/AMDGPU/waitcnt-unscoped.ll b/llvm/test/CodeGen/AMDGPU/waitcnt-unscoped.ll
new file mode 100644
index 0000000000000..6d24de85a8ad8
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/waitcnt-unscoped.ll
@@ -0,0 +1,65 @@
+; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=gfx950 < %s | FileCheck -check-prefix=CHECK %s
+
+declare void @llvm.amdgcn.sched.barrier(i32 %mask) #0
+declare void @llvm.amdgcn.load.to.lds(ptr %in, ptr addrspace(3) %lds_out, i32 %size, i32 %offset, i32 %aux) #0
+
+define amdgpu_kernel void @test_waitcnt(ptr addrspace(1) %global_buffer, ptr addrspace(3) %lds_buffer1, ptr addrspace(3) %lds_buffer2) {
+; This test checks if SIInsertWaitcnts pass inserts S_WAITCNT VMCNT(0) before DS_READ
+; CHECK-NOT: s_waitcnt vmcnt(0)
+; CHECK: ds_read_b32
+entry:
+ ; VMEM accesses with alias.scope
+ %vmem_load = load i32, ptr addrspace(1) %global_buffer
+ %gepvmem = getelementptr i32, ptr addrspace(1) %global_buffer, i32 16
+ store i32 %vmem_load, ptr addrspace(1) %gepvmem, align 4, !alias.scope !0
+
+ ; Global to LDS load
+ %gepvmem.ascast = addrspacecast ptr addrspace(1) %gepvmem to ptr
+ call void @llvm.amdgcn.load.to.lds(ptr %gepvmem.ascast, ptr addrspace(3) %lds_buffer1, i32 4, i32 4, i32 0), !alias.scope !9, !noalias !14
+
+ ; Insert scheduling barrier
+ call void @llvm.amdgcn.sched.barrier(i32 0)
+
+ ; DS_WRITEs with alias.scope and noalias
+ store i32 %vmem_load, ptr addrspace(3) %lds_buffer1, align 4, !alias.scope !1, !noalias !12
+ store i32 %vmem_load, ptr addrspace(3) %lds_buffer2, align 4, !alias.scope !6, !noalias !13
+
+ ; Insert scheduling barrier
+ call void @llvm.amdgcn.sched.barrier(i32 0)
+
+ ; DS_READ with alias.scope missing
+ %lds_load = load i32, ptr addrspace(3) %lds_buffer1, align 4, !noalias !12
+
+ ; VMEM write
+ %gep = getelementptr i32, ptr addrspace(1) %global_buffer, i32 4
+ %gep2 = getelementptr i32, ptr addrspace(1) %global_buffer, i32 8
+ store i32 %lds_load, ptr addrspace(1) %gep, align 4, !alias.scope !0
+ store i32 %vmem_load, ptr addrspace(1) %gep2, align 4, !alias.scope !0
+
+ ret void
+}
+
+; VMEM alias domain and scope
+!5 = !{!"vmem.domain"}
+!4 = !{!"vmem.scope", !5}
+!0 = !{!4}
+
+; LDS alias domains and scopes
+!3 = !{!"lds1.domain"}
+!2 = !{!"lds1.scope", !3}
+!1 = !{!2}
+
+!8 = !{!"lds2.domain"}
+!7 = !{!"lds2.scope", !8}
+!6 = !{!7}
+
+!11 = !{!"lds1_off4.domain"}
+!10 = !{!"lds1_off4.scope", !11}
+!9 = !{!10}
+
+; Noalias lists
+!12 = !{!7, !10}
+!13 = !{!2, !10}
+!14 = !{!2, !7}
+
+attributes #0 = { nounwind }
|
arsenm
reviewed
Sep 10, 2025
arsenm
reviewed
Sep 10, 2025
arsenm
reviewed
Sep 10, 2025
arsenm
reviewed
Sep 10, 2025
rampitec
reviewed
Sep 10, 2025
choikwa
added a commit
to choikwa/llvm-project
that referenced
this pull request
Sep 10, 2025
…nstBefore Pre-commit testcase for llvm#157821
…efore This change was motivated by CK where many VMCNT(0)'s were generated due to instructions lacking !alias.scope metadata. The two causes of this were 1) LowerLDSModule not tacking on scope metadata on a single LDS variable 2) IPSCCP pass before inliner replacing noalias ptr derivative with a global value, which made inliner unable to track it back to the noalias ptr argument. However, it turns out that IPSCCP losing the scope information was largely ineffectual as ScopedNoAliasAA was able to handle asymmetric condition, where one MemLoc was missing scope, and still return NoAlias result. AMDGPU however was checking for existence of scope in SIInsertWaitcnts and conservatively treating it as aliasing all and inserted VMCNT(0) before DS_READs, forcing it to wait for all previous LDS DMA instructions. Since we know that ScopedNoAliasAA can handle asymmetry, we should also allow AA query to determine if two MIs may alias. Remove confusing comments, redundant check-prefix, move function attribute turn into positive checks via script
choikwa
force-pushed
the
siinsertwaitcnts-remove-scope
branch
from
436fb0e to
55b34c2
Compare
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Sep 10, 2025
…ateWaitcntInstBefore (#157938) Pre-commit testcase for llvm/llvm-project#157821
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/24122 Here is the relevant piece of the build log for the reference |
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Sep 25, 2025
…nstBefore (llvm#157938) Pre-commit testcase for llvm#157821
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Sep 25, 2025
…efore (llvm#157821) This change was motivated by CK where many VMCNT(0)'s were generated due to instructions lacking !alias.scope metadata. The two causes of this were: 1) LowerLDSModule not tacking on scope metadata on a single LDS variable 2) IPSCCP pass before inliner replacing noalias ptr derivative with a global value, which made inliner unable to track it back to the noalias ptr argument. However, it turns out that IPSCCP losing the scope information was largely ineffectual as ScopedNoAliasAA was able to handle asymmetric condition, where one MemLoc was missing scope, and still return NoAlias result. AMDGPU however was checking for existence of scope in SIInsertWaitcnts and conservatively treating it as aliasing all and inserted VMCNT(0) before DS_READs, forcing it to wait for all previous LDS DMA instructions. Since we know that ScopedNoAliasAA can handle asymmetry, we should also allow AA query to determine if two MIs may alias. Passed PSDB. Previous attempt to address the issue in IPSCCP, likely stalled: llvm#154522 This solution may be preferrable over that as issue only affects AMDGPU. Cherry-picked from 8ae3aea and ef7de8d
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Sep 25, 2025
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Oct 9, 2025
…nstBefore (llvm#157938) Pre-commit testcase for llvm#157821
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Oct 9, 2025
…efore (llvm#157821) This change was motivated by CK where many VMCNT(0)'s were generated due to instructions lacking !alias.scope metadata. The two causes of this were: 1) LowerLDSModule not tacking on scope metadata on a single LDS variable 2) IPSCCP pass before inliner replacing noalias ptr derivative with a global value, which made inliner unable to track it back to the noalias ptr argument. However, it turns out that IPSCCP losing the scope information was largely ineffectual as ScopedNoAliasAA was able to handle asymmetric condition, where one MemLoc was missing scope, and still return NoAlias result. AMDGPU however was checking for existence of scope in SIInsertWaitcnts and conservatively treating it as aliasing all and inserted VMCNT(0) before DS_READs, forcing it to wait for all previous LDS DMA instructions. Since we know that ScopedNoAliasAA can handle asymmetry, we should also allow AA query to determine if two MIs may alias. Passed PSDB. Previous attempt to address the issue in IPSCCP, likely stalled: llvm#154522 This solution may be preferrable over that as issue only affects AMDGPU.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change was motivated by CK where many VMCNT(0)'s were generated due to instructions lacking !alias.scope metadata. The two causes of this were:
global value, which made inliner unable to track it back to the noalias
ptr argument.
However, it turns out that IPSCCP losing the scope information was largely ineffectual as ScopedNoAliasAA was able to handle asymmetric condition, where one MemLoc was missing scope, and still return NoAlias result.
AMDGPU however was checking for existence of scope in SIInsertWaitcnts and conservatively treating it as aliasing all and inserted VMCNT(0) before DS_READs, forcing it to wait for all previous LDS DMA instructions.
Since we know that ScopedNoAliasAA can handle asymmetry, we should also allow AA query to determine if two MIs may alias.
Passed PSDB.
Previous attempt to address the issue in IPSCCP, likely stalled: #154522
This solution may be preferrable over that as issue only affects AMDGPU.