Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Lint][AMDGPU] No store to const addrspace #109181

Merged
merged 4 commits into from
Sep 25, 2024
Merged

[Lint][AMDGPU] No store to const addrspace #109181

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a099028
[Verifier][AMDGPU] No store to const addrspace
jofrn Sep 18, 2024
4553484
[Lint] Switch check to Linter
jofrn Sep 20, 2024
393311c
Add negative test, move isConstantAddressSpace
jofrn Sep 20, 2024
ae211fa
Added cmpxchg, atomicrmw, call to Lint
jofrn Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions llvm/lib/IR/Verifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4224,6 +4224,33 @@ void Verifier::visitLoadInst(LoadInst &LI) {
visitInstruction(LI);
}

static bool isConstantAddressSpace(unsigned AS) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constant address space is not an AMD only thing. I'm thinking probably we can split this function into target dependent (for example, to query whether an AS is a constant AS) and independent part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would the independent part contain if we do not have a use case for it yet?

switch (AS) {
using namespace AMDGPUAS;
case CONSTANT_ADDRESS:
case CONSTANT_ADDRESS_32BIT:
case CONSTANT_BUFFER_0:
case CONSTANT_BUFFER_1:
case CONSTANT_BUFFER_2:
case CONSTANT_BUFFER_3:
case CONSTANT_BUFFER_4:
case CONSTANT_BUFFER_5:
case CONSTANT_BUFFER_6:
case CONSTANT_BUFFER_7:
case CONSTANT_BUFFER_8:
case CONSTANT_BUFFER_9:
case CONSTANT_BUFFER_10:
case CONSTANT_BUFFER_11:
case CONSTANT_BUFFER_12:
case CONSTANT_BUFFER_13:
case CONSTANT_BUFFER_14:
case CONSTANT_BUFFER_15:
return true;
default:
return false;
}
}

void Verifier::visitStoreInst(StoreInst &SI) {
PointerType *PTy = dyn_cast<PointerType>(SI.getOperand(1)->getType());
Check(PTy, "Store operand must be a pointer.", &SI);
Expand All @@ -4246,6 +4273,10 @@ void Verifier::visitStoreInst(StoreInst &SI) {
Check(SI.getSyncScopeID() == SyncScope::System,
"Non-atomic store cannot have SynchronizationScope specified", &SI);
}
if (TT.isAMDGPU()) {
Check(!isConstantAddressSpace(SI.getPointerAddressSpace()),
"Store cannot be to constant addrspace", &SI);
}
visitInstruction(SI);
}

Expand Down
9 changes: 9 additions & 0 deletions llvm/test/CodeGen/AMDGPU/const-store.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
; RUN: not llc -mtriple=amdgcn < %s |& FileCheck %s
jofrn marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

negative test with a different target

define amdgpu_kernel void @store_const(ptr addrspace(4) %out, i32 %a, i32 %b) {
; CHECK: Store cannot be to constant addrspace
; CHECK-NEXT: store i32 %r, ptr addrspace(4) %out
%r = add i32 %a, %b
store i32 %r, ptr addrspace(4) %out
ret void
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test atomicrmw, cmpxchg, and a memset, memcpy? Also at least the addrspace(6) case

Loading