[AMDGPU] Enable i8 GEP promotion for vector allocas #166132
Merged
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
harrisonGPU
requested review from
arsenm,
jayfoad,
perlfu,
ritter-x2a,
ruiling and
shiltian
Member
|
@llvm/pr-subscribers-backend-amdgpu Author: Harrison Hao (harrisonGPU) ChangesThis patch adds support for the pattern: %elt = getelementptr inbounds i8, ptr addrspace(5) %alloca, i32 %indexby scaling the byte offset to an element index (index >> log2(ElemSize)), Full diff: https://github.com/llvm/llvm-project/pull/166132.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp b/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
index ddabd25894414..793c0237cdf38 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
@@ -456,10 +456,21 @@ static Value *GEPToVectorIndex(GetElementPtrInst *GEP, AllocaInst *Alloca,
const auto &VarOffset = VarOffsets.front();
APInt OffsetQuot;
APInt::sdivrem(VarOffset.second, VecElemSize, OffsetQuot, Rem);
- if (Rem != 0 || OffsetQuot.isZero())
- return nullptr;
+
+ Value *Scaled = nullptr;
+ if (Rem != 0 || OffsetQuot.isZero()) {
+ unsigned ElemSizeShift = Log2_64(VecElemSize);
+ Scaled = Builder.CreateLShr(VarOffset.first, ElemSizeShift);
+ if (Instruction *NewInst = dyn_cast<Instruction>(Scaled))
+ NewInsts.push_back(NewInst);
+ OffsetQuot = APInt(BW, 1);
+ Rem = 0;
+ }
Value *Offset = VarOffset.first;
+ if (Scaled)
+ Offset = Scaled;
+
auto *OffsetType = dyn_cast<IntegerType>(Offset->getType());
if (!OffsetType)
return nullptr;
diff --git a/llvm/test/CodeGen/AMDGPU/promote-alloca-vector-gep.ll b/llvm/test/CodeGen/AMDGPU/promote-alloca-vector-gep.ll
index 76e1868b3c4b9..65bddaba8dd14 100644
--- a/llvm/test/CodeGen/AMDGPU/promote-alloca-vector-gep.ll
+++ b/llvm/test/CodeGen/AMDGPU/promote-alloca-vector-gep.ll
@@ -250,6 +250,26 @@ bb2:
store i32 0, ptr addrspace(5) %extractelement
ret void
}
+
+define amdgpu_kernel void @scalar_alloca_vector_gep_i8(ptr %buffer, float %data, i32 %index) {
+; CHECK-LABEL: define amdgpu_kernel void @scalar_alloca_vector_gep_i8(
+; CHECK-SAME: ptr [[BUFFER:%.*]], float [[DATA:%.*]], i32 [[INDEX:%.*]]) {
+; CHECK-NEXT: [[ALLOCA:%.*]] = freeze <3 x float> poison
+; CHECK-NEXT: [[VEC:%.*]] = load <3 x float>, ptr [[BUFFER]], align 16
+; CHECK-NEXT: [[TMP1:%.*]] = lshr i32 [[INDEX]], 2
+; CHECK-NEXT: [[TMP2:%.*]] = insertelement <3 x float> [[VEC]], float [[DATA]], i32 [[TMP1]]
+; CHECK-NEXT: store <3 x float> [[TMP2]], ptr [[BUFFER]], align 16
+; CHECK-NEXT: ret void
+;
+ %alloca = alloca <3 x float>, align 16, addrspace(5)
+ %vec = load <3 x float>, ptr %buffer
+ store <3 x float> %vec, ptr addrspace(5) %alloca
+ %elt = getelementptr inbounds nuw i8, ptr addrspace(5) %alloca, i32 %index
+ store float %data, ptr addrspace(5) %elt, align 4
+ %updated = load <3 x float>, ptr addrspace(5) %alloca, align 16
+ store <3 x float> %updated, ptr %buffer, align 16
+ ret void
+}
;.
; CHECK: [[META0]] = !{}
; CHECK: [[RNG1]] = !{i32 0, i32 1025}
|
shiltian
reviewed
Nov 3, 2025
Contributor
shiltian
left a comment
shiltian
left a comment
There was a problem hiding this comment.
What if the index here is somewhere in the middle? It doesn't look like we have any constraint on the index.
arsenm
reviewed
Nov 4, 2025
|
|
||
| Value *Scaled = nullptr; | ||
| if (Rem != 0 || OffsetQuot.isZero()) { | ||
| unsigned ElemSizeShift = Log2_64(VecElemSize); |
Contributor
There was a problem hiding this comment.
Need to validate VecElemSize is a power of 2?
Contributor
Author
There was a problem hiding this comment.
Thanks, but I think it is not necessary to explicitly check whether the element size is a power of two, because it is already covered by the existing check here:
llvm-project/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
Lines 871 to 877 in 52fdcf9
If the element type is not naturally aligned, it will return false, which also rejects non power of 2 element sizes, such as i24.
| %updated = load <3 x float>, ptr addrspace(5) %alloca, align 16 | ||
| store <3 x float> %updated, ptr %buffer, align 16 | ||
| ret void | ||
| } |
Contributor
There was a problem hiding this comment.
Test with non-power-of-2 element size
Contributor
Author
Could you please give me an example? |
Contributor
In the test case, the |
harrisonGPU
force-pushed
the
amdgpu/promote-vector
branch
from
c1439a3 to
6a28740
Compare
Contributor
Author
Hi @shiltian , I’ve already thought about this issue, thank you very much for your suggestion and for pointing it out. |
shiltian
reviewed
Nov 18, 2025
| return nullptr; | ||
|
|
||
| Value *Offset = VarOffset.first; | ||
| if (Rem != 0 || OffsetQuot.isZero()) { |
Contributor
There was a problem hiding this comment.
Does it have to be this complicated? I thought checking whether offset % size would be sufficient?
Contributor
Author
There was a problem hiding this comment.
Thanks, I agree your points, I have removed it.
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
Contributor
|
The logic seems reasonable for plain vector, but I'm not sure about a nested vector. Can you add tests as well? |
Contributor
Author
Thanks, I have added two tests about nested vector. |
shiltian
approved these changes
Dec 8, 2025
harrisonGPU
force-pushed
the
amdgpu/promote-vector
branch
from
8c3f2e3 to
0e8c3fe
Compare
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/18635 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/23325 Here is the relevant piece of the build log for the reference |
jplehr
added a commit
that referenced
this pull request
Dec 8, 2025
Reverts #166132 Broke libc on GPU tests. https://lab.llvm.org/buildbot/#/builders/10/builds/18635
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Dec 8, 2025
…s" (#171087) Reverts llvm/llvm-project#166132 Broke libc on GPU tests. https://lab.llvm.org/buildbot/#/builders/10/builds/18635
honeygoyal
pushed a commit
to honeygoyal/llvm-project
that referenced
this pull request
Dec 9, 2025
This patch adds support for the pattern: ```llvm %index = select i1 %idx_sel, i32 0, i32 4 %elt = getelementptr inbounds i8, ptr addrspace(5) %alloca, i32 %index ``` by scaling the byte offset to an element index (index >> log2(ElemSize)), allowing the vector element to be updated with insertelement instead of using scratch memory.
honeygoyal
pushed a commit
to honeygoyal/llvm-project
that referenced
this pull request
Dec 9, 2025
…1087) Reverts llvm#166132 Broke libc on GPU tests. https://lab.llvm.org/buildbot/#/builders/10/builds/18635
ruiling
reviewed
Dec 10, 2025
| NewInsts.push_back(NewInst); | ||
|
|
||
| Offset = Scaled; | ||
| OffsetQuot = APInt(BW, 1); |
Contributor
There was a problem hiding this comment.
Sorry look into this very late. This is wrong. The case we want to optimize is when (VarOffset.first * VarOffset.second) % VecElemSize == 0, but VarOffset.second % VecElemSize != 0. To calculate the vector index, you need (VarOffset.first * VarOffset.second / VecElemSize). Here you reset OffsetQuot to one. So you were actually dropping the VarOffset.second. I can see the change here will have conflict with #170512. As the code below was mostly moved away. And the argument NewInsts was also removed. I would like we do some further refactor based on #170512. Rename the function GEPToVectorIndex to isPtrOffsetAlignedToElementSize(). And just return the three components if the offset is properly aligned. And do vector index calculation later.
| %vec = load <3 x float>, ptr %buffer | ||
| store <3 x float> %vec, ptr addrspace(5) %alloca | ||
| %index = select i1 %idx_sel, i32 4, i32 8 | ||
| %elt = getelementptr inbounds nuw i8, ptr addrspace(5) %alloca, i32 %index |
Contributor
There was a problem hiding this comment.
It would be better you change this to gep of i16, so we have a case the multiplier of the variable offset part is not 1.
| ret void | ||
| } | ||
|
|
||
| define amdgpu_kernel void @scalar_alloca_vector_gep_i8_4_or_5_no_promote(ptr %buffer, float %data, i1 %idx_sel) { |
Contributor
There was a problem hiding this comment.
Maybe switch to another calling convention so that the alloca will be kept unchanged (not being promoted to LDS)?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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 patch adds support for the pattern:
by scaling the byte offset to an element index (index >> log2(ElemSize)),
allowing the vector element to be updated with insertelement instead of using
scratch memory.