[AMDGPU] Apply alignment attr for make.buffer.rsrc #166914
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
Shoreshen
requested review from
arsenm and
shiltian
and removed request for
arsenm
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-amdgpu Author: None (Shoreshen) ChangesCalculating alignment for For example: define float @<!-- -->foo(ptr addrspace(1) align X %ptr) {
%fat.ptr = call ptr addrspace(7) @<!-- -->llvm.amdgcn.make.buffer.rsrc.p7.p1(ptr addrspace(1) %ptr, i16 0, i32 C, i32 0)
%y = load float, ptr addrspace(7) %fat.ptr, align Y
ret float %y
}It hopes that Full diff: https://github.com/llvm/llvm-project/pull/166914.diff 4 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp b/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
index 56ab040706a13..70f2fbae08ada 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
@@ -1603,7 +1603,7 @@ static bool runImpl(Module &M, AnalysisGetter &AG, TargetMachine &TM,
&AAAMDGPUMinAGPRAlloc::ID, &AACallEdges::ID, &AAPointerInfo::ID,
&AAPotentialConstantValues::ID, &AAUnderlyingObjects::ID,
&AANoAliasAddrSpace::ID, &AAAddressSpace::ID, &AAIndirectCallInfo::ID,
- &AAAMDGPUClusterDims::ID});
+ &AAAMDGPUClusterDims::ID, &AAAlign::ID});
AttributorConfig AC(CGUpdater);
AC.IsClosedWorldModule = Options.IsClosedWorld;
@@ -1657,6 +1657,9 @@ static bool runImpl(Module &M, AnalysisGetter &AG, TargetMachine &TM,
Ptr = RMW->getPointerOperand();
else if (auto *CmpX = dyn_cast<AtomicCmpXchgInst>(&I))
Ptr = CmpX->getPointerOperand();
+ else if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(&I))
+ if (II->getIntrinsicID() == Intrinsic::amdgcn_make_buffer_rsrc)
+ A.getOrCreateAAFor<AAAlign>(IRPosition::value(*II));
if (Ptr) {
A.getOrCreateAAFor<AAAddressSpace>(IRPosition::value(*Ptr));
diff --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
index a6ac7610a2c7a..37ff282343889 100644
--- a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
+++ b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
@@ -5279,6 +5279,12 @@ struct AAAlignImpl : AAAlign {
/// See AbstractAttribute::initialize(...).
void initialize(Attributor &A) override {
+ // For make.buffer.rsrc, the alignment strictly equals to the base's
+ // alignment
+ if (Instruction *I = dyn_cast<Instruction>(&getAssociatedValue()))
+ if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I))
+ if (II->getIntrinsicID() == Intrinsic::amdgcn_make_buffer_rsrc)
+ return;
SmallVector<Attribute, 4> Attrs;
A.getAttrs(getIRPosition(), {Attribute::Alignment}, Attrs);
for (const Attribute &Attr : Attrs)
@@ -5300,10 +5306,19 @@ struct AAAlignImpl : AAAlign {
if (isa<ConstantData>(AssociatedValue))
return ChangeStatus::UNCHANGED;
+ // For use of amdgcn.make.buffer.rsrc, the alignment equals to
+ // min(base, load/store)
+ bool IsMakeBufferRsrc = false;
+ if (Instruction *I = dyn_cast<Instruction>(&getAssociatedValue()))
+ if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I))
+ if (II->getIntrinsicID() == Intrinsic::amdgcn_make_buffer_rsrc)
+ IsMakeBufferRsrc = true;
for (const Use &U : AssociatedValue.uses()) {
if (auto *SI = dyn_cast<StoreInst>(U.getUser())) {
if (SI->getPointerOperand() == &AssociatedValue)
- if (SI->getAlign() < getAssumedAlign()) {
+ if (IsMakeBufferRsrc) {
+ SI->setAlignment(std::min(SI->getAlign(), getAssumedAlign()));
+ } else if (SI->getAlign() < getAssumedAlign()) {
STATS_DECLTRACK(AAAlign, Store,
"Number of times alignment added to a store");
SI->setAlignment(getAssumedAlign());
@@ -5311,14 +5326,18 @@ struct AAAlignImpl : AAAlign {
}
} else if (auto *LI = dyn_cast<LoadInst>(U.getUser())) {
if (LI->getPointerOperand() == &AssociatedValue)
- if (LI->getAlign() < getAssumedAlign()) {
+ if (IsMakeBufferRsrc) {
+ LI->setAlignment(std::min(LI->getAlign(), getAssumedAlign()));
+ } else if (LI->getAlign() < getAssumedAlign()) {
LI->setAlignment(getAssumedAlign());
STATS_DECLTRACK(AAAlign, Load,
"Number of times alignment added to a load");
InstrChanged = ChangeStatus::CHANGED;
}
} else if (auto *RMW = dyn_cast<AtomicRMWInst>(U.getUser())) {
- if (RMW->getPointerOperand() == &AssociatedValue) {
+ if (IsMakeBufferRsrc) {
+ RMW->setAlignment(std::min(RMW->getAlign(), getAssumedAlign()));
+ } else if (RMW->getPointerOperand() == &AssociatedValue) {
if (RMW->getAlign() < getAssumedAlign()) {
STATS_DECLTRACK(AAAlign, AtomicRMW,
"Number of times alignment added to atomicrmw");
@@ -5328,7 +5347,9 @@ struct AAAlignImpl : AAAlign {
}
}
} else if (auto *CAS = dyn_cast<AtomicCmpXchgInst>(U.getUser())) {
- if (CAS->getPointerOperand() == &AssociatedValue) {
+ if (IsMakeBufferRsrc) {
+ CAS->setAlignment(std::min(CAS->getAlign(), getAssumedAlign()));
+ } else if (CAS->getPointerOperand() == &AssociatedValue) {
if (CAS->getAlign() < getAssumedAlign()) {
STATS_DECLTRACK(AAAlign, AtomicCmpXchg,
"Number of times alignment added to cmpxchg");
@@ -5554,6 +5575,15 @@ struct AAAlignCallSiteReturned final
std::min(this->getAssumedAlign(), Alignment).value());
break;
}
+ case Intrinsic::amdgcn_make_buffer_rsrc: {
+ const auto *AlignAA =
+ A.getAAFor<AAAlign>(*this, IRPosition::value(*(II->getOperand(0))),
+ DepClassTy::REQUIRED);
+ if (AlignAA && AlignAA->isValidState())
+ return clampStateAndIndicateChange<StateType>(
+ this->getState(), AlignAA->getAssumedAlign().value());
+ break;
+ }
default:
break;
}
diff --git a/llvm/test/CodeGen/AMDGPU/attr-amdgpu-align.ll b/llvm/test/CodeGen/AMDGPU/attr-amdgpu-align.ll
new file mode 100644
index 0000000000000..8d2bfab09460b
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/attr-amdgpu-align.ll
@@ -0,0 +1,26 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -passes=amdgpu-attributor %s -o - | FileCheck %s
+
+define float @load_gt_base(ptr align 4 %p) {
+; CHECK-LABEL: define float @load_gt_base(
+; CHECK-SAME: ptr align 4 [[P:%.*]]) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT: [[PTR:%.*]] = call align 4 ptr addrspace(7) @llvm.amdgcn.make.buffer.rsrc.p7.p0(ptr align 4 [[P]], i16 0, i64 0, i32 0)
+; CHECK-NEXT: [[LOADED:%.*]] = load float, ptr addrspace(7) [[PTR]], align 4
+; CHECK-NEXT: ret float [[LOADED]]
+;
+ %ptr = call ptr addrspace(7) @llvm.amdgcn.make.buffer.rsrc.p7.p0(ptr %p, i16 0, i64 0, i32 0)
+ %loaded = load float, ptr addrspace(7) %ptr, align 8
+ ret float %loaded
+}
+
+define float @load_lt_base(ptr align 8 %p) {
+; CHECK-LABEL: define float @load_lt_base(
+; CHECK-SAME: ptr align 8 [[P:%.*]]) #[[ATTR0]] {
+; CHECK-NEXT: [[PTR:%.*]] = call align 8 ptr addrspace(7) @llvm.amdgcn.make.buffer.rsrc.p7.p0(ptr align 8 [[P]], i16 0, i64 0, i32 0)
+; CHECK-NEXT: [[LOADED:%.*]] = load float, ptr addrspace(7) [[PTR]], align 4
+; CHECK-NEXT: ret float [[LOADED]]
+;
+ %ptr = call ptr addrspace(7) @llvm.amdgcn.make.buffer.rsrc.p7.p0(ptr %p, i16 0, i64 0, i32 0)
+ %loaded = load float, ptr addrspace(7) %ptr, align 4
+ ret float %loaded
+}
diff --git a/llvm/test/Transforms/Attributor/AMDGPU/tag-invariant-loads.ll b/llvm/test/Transforms/Attributor/AMDGPU/tag-invariant-loads.ll
index 1ab607465dbbb..34bbfa8974747 100644
--- a/llvm/test/Transforms/Attributor/AMDGPU/tag-invariant-loads.ll
+++ b/llvm/test/Transforms/Attributor/AMDGPU/tag-invariant-loads.ll
@@ -306,7 +306,7 @@ define amdgpu_kernel void @test_call_untouched_ptr() {
define amdgpu_kernel void @test_make_buffer(ptr addrspace(1) %ptr) {
; AMDGCN-LABEL: define amdgpu_kernel void @test_make_buffer(
; AMDGCN-SAME: ptr addrspace(1) nofree readonly captures(none) [[PTR:%.*]]) #[[ATTR2]] {
-; AMDGCN-NEXT: [[RSRC:%.*]] = call align 4 ptr addrspace(7) @llvm.amdgcn.make.buffer.rsrc.p7.p1(ptr addrspace(1) [[PTR]], i16 noundef 0, i64 noundef 0, i32 noundef 0) #[[ATTR11:[0-9]+]]
+; AMDGCN-NEXT: [[RSRC:%.*]] = call ptr addrspace(7) @llvm.amdgcn.make.buffer.rsrc.p7.p1(ptr addrspace(1) [[PTR]], i16 noundef 0, i64 noundef 0, i32 noundef 0) #[[ATTR11:[0-9]+]]
; AMDGCN-NEXT: [[VAL:%.*]] = load i32, ptr addrspace(7) [[RSRC]], align 4
; AMDGCN-NEXT: call void @clobber(i32 [[VAL]]) #[[ATTR7]]
; AMDGCN-NEXT: ret void
@@ -321,7 +321,7 @@ define amdgpu_kernel void @test_make_buffer(ptr addrspace(1) %ptr) {
define amdgpu_kernel void @test_make_buffer_noalias(ptr addrspace(1) noalias %ptr) {
; AMDGCN-LABEL: define amdgpu_kernel void @test_make_buffer_noalias(
; AMDGCN-SAME: ptr addrspace(1) noalias nofree readonly captures(none) [[PTR:%.*]]) #[[ATTR2]] {
-; AMDGCN-NEXT: [[RSRC:%.*]] = call align 4 ptr addrspace(7) @llvm.amdgcn.make.buffer.rsrc.p7.p1(ptr addrspace(1) [[PTR]], i16 noundef 0, i64 noundef 0, i32 noundef 0) #[[ATTR11]]
+; AMDGCN-NEXT: [[RSRC:%.*]] = call ptr addrspace(7) @llvm.amdgcn.make.buffer.rsrc.p7.p1(ptr addrspace(1) [[PTR]], i16 noundef 0, i64 noundef 0, i32 noundef 0) #[[ATTR11]]
; AMDGCN-NEXT: [[VAL:%.*]] = load i32, ptr addrspace(7) [[RSRC]], align 4, !invariant.load [[META0]]
; AMDGCN-NEXT: call void @clobber(i32 [[VAL]]) #[[ATTR7]]
; AMDGCN-NEXT: ret void
|
shiltian
requested changes
Nov 7, 2025
Contributor
shiltian
left a comment
shiltian
left a comment
There was a problem hiding this comment.
I don't think we should put the handling of AMDGPU specific code into the generic code. We can potentially create a class inheriting the existing ones in AMDGPUAttributor dedicated for the handling of AMDGPU specific stuff.
Contributor
AMDGPUAttributor isn't really the place for it either. It's not an AMDGPU specific attribute |
Contributor
|
Title should make it clear this is about inferring it, not changing the intrinsic definition |
arsenm
reviewed
Nov 7, 2025
| const auto *AlignAA = | ||
| A.getAAFor<AAAlign>(*this, IRPosition::value(*(II->getOperand(0))), | ||
| DepClassTy::REQUIRED); | ||
| if (AlignAA && AlignAA->isValidState()) |
Contributor
There was a problem hiding this comment.
Does TargetTransformInfo have some kind of alignment propagation already? I thought it did
Contributor
Author
There was a problem hiding this comment.
Hi @arsenm , I had a look but cannot be sure. There are lots of align related function but mainly used for legality check and cost computation.
I'm not really familiar with the struct, if you could be more specific?? So that I can have some directions to search~~
Thanks
Contributor
It is an AMDGPU specific intrinsic. |
Contributor
|
I'd like to flag #167553 as related (and to make sure we're on the same page as to what we mean by |
shiltian
reviewed
Nov 12, 2025
| llvm_unreachable("AAAMDGPUClusterDims is only valid for function position"); | ||
| } | ||
|
|
||
| struct AAAMDGPUMakeBufferRsrcAlign |
Contributor
There was a problem hiding this comment.
I'd make it something like AAAMDGPUAlign, and then use it to deal with all AMDGPU related alignments.
Contributor
There was a problem hiding this comment.
Also, document the new class.
Contributor
That does not make the transform AMDGPU specific. This is applying target intrinsic knowledge to a generic intrinsic. The handling should be directly in AAAlign. I would rank handling it as an AMDGPU specific Attributor significantly worse than just hardcoding the target intrinsic in AAAlign. Better would be to have TTI handle the relevant parsing |
Contributor
|
We probably could use the TTI information from #140802 to get the alignment |
krzysz00
reviewed
Nov 12, 2025
Contributor
krzysz00
left a comment
krzysz00
left a comment
There was a problem hiding this comment.
I'm with @arsenm on "this goes in the generic align handling"
Overall, can we see a few more test cases - a load/store that is underaligned and a function argument with aligns on it, for example.
I'd also like to see (especially after my docs PR lands) a note somewhere in the code that this is a conservatively correct approach to this alignment inference.
Contributor
|
The pointer operand could be a function argument or a GEP result or ... anything you can do to a pointer, really. I think the simplest thing to do would be to say that Therefore, if I have a So I think that adding generic handling for "this returns an existing pointer without capturing" might be the right call |
Contributor
In the previous PR that handles the |
Contributor
|
The one question is do we want the following define float @overaligned(ptr addrspace(1) align(2) inreg %x, i32 inreg %off) {
%rsrc = call ptr addrspace(7) @llvm.amdgcn.make.buffer.rsrc(%x, ...)
%spot = getelementptr i8, ptr addrspace(7) %rsrc, i32 %off
;;; UB! This load is "align 4" but the base pointer is only known to be aligned to 2
;;; Because both the base and `%off` are workgroup-uniform, this `load` is (though we don't do
;;; this atm) promotable to s_buffer_load, where the last two bits of `%x`'s address would be silently
;;; dropped.
%ret = load float, ptr addrspace(7) %spot, align 4, !invariant.load
ret float %ret
}to just stay as UB or do we want to fix it to be |
Contributor
|
The attributor will change the alignment of |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
shiltian
reviewed
Dec 1, 2025
| A.getAAFor<AAAlign>(*this, IRPosition::value(*(II->getOperand(0))), | ||
| DepClassTy::REQUIRED); | ||
| if (AlignAA && AlignAA->isValidState()) { | ||
| if (AlignAA) { |
Contributor
There was a problem hiding this comment.
not sure about this change
Contributor
Author
There was a problem hiding this comment.
Hi @shiltian , if I'm not wrong the AAAlign would be invalid if assumed alignment is 1:
/// See AbstractState::isValidState()
/// NOTE: For now we simply pretend that the worst possible state is invalid.
bool isValidState() const override { return Assumed != getWorstState(); }In this case, if mask is not valid, then assumed alignment of ptrmask should also be 1...
Co-authored-by: Shilei Tian <i@tianshilei.me>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
shiltian
reviewed
Dec 4, 2025
| return AlignAA->getKnownAlign().value(); | ||
| break; | ||
| } | ||
| case Intrinsic::amdgcn_make_buffer_rsrc: { |
Contributor
There was a problem hiding this comment.
add a comment here regarding the handling of target specific handling and some future direction that we might take.
shiltian
approved these changes
Dec 5, 2025
Co-authored-by: Shilei Tian <i@tianshilei.me>
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/34534 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/169/builds/17785 Here is the relevant piece of the build log for the reference |
honeygoyal
pushed a commit
to honeygoyal/llvm-project
that referenced
this pull request
Dec 9, 2025
Calculating alignment for `make.buffer.rsrc` intrinsic. The logic is the alignment on use of return value of `make.buffer.rsrc` should be capped by the base operand's alignment of `make.buffer.rsrc`. For example: ```ll define float @foo(ptr addrspace(1) align X %ptr) { %fat.ptr = call ptr addrspace(7) @llvm.amdgcn.make.buffer.rsrc.p7.p1(ptr addrspace(1) %ptr, i16 0, i32 C, i32 0) %y = load float, ptr addrspace(7) %fat.ptr, align Y ret float %y } ``` We hopes that `Y = min(X, Y)` --- After discussion, it seems improper for letting `Y = min(X, Y)` since it contradict with the semantic of align on load. So we would apply the origin behavior of align, which is letting `X` and `Y` both equal to `max(X, Y)` --------- Co-authored-by: Shilei Tian <i@tianshilei.me>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 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.
Calculating alignment for
make.buffer.rsrcintrinsic. The logic is the alignment on use of return value ofmake.buffer.rsrcshould be capped by the base operand's alignment ofmake.buffer.rsrc.For example:
We hopes that
Y = min(X, Y)After discussion, it seems improper for letting
Y = min(X, Y)since it contradict with the semantic of align on load.So we would apply the origin behavior of align, which is letting
XandYboth equal tomax(X, Y)