Skip to content

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

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

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 all commits
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
27 changes: 27 additions & 0 deletions llvm/include/llvm/Support/AMDGPUAddrSpace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,33 @@ inline bool isExtendedGlobalAddrSpace(unsigned AS) {
AS == AMDGPUAS::CONSTANT_ADDRESS_32BIT ||
AS > AMDGPUAS::MAX_AMDGPU_ADDRESS;
}

inline bool isConstantAddressSpace(unsigned AS) {
switch (AS) {
using namespace AMDGPUAS;
Comment on lines +98 to +99
Copy link
Contributor

@arsenm arsenm Sep 24, 2024

Choose a reason for hiding this comment

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

Suggested change
switch (AS) {
using namespace AMDGPUAS;
using namespace AMDGPUAS;
switch (AS) {

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;
}
}
} // end namespace AMDGPU

} // end namespace llvm
Expand Down
23 changes: 21 additions & 2 deletions llvm/lib/Analysis/Lint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
#include "llvm/IR/PassManager.h"
#include "llvm/IR/Type.h"
#include "llvm/IR/Value.h"
#include "llvm/Support/AMDGPUAddrSpace.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/KnownBits.h"
#include "llvm/Support/raw_ostream.h"
Expand Down Expand Up @@ -102,6 +103,8 @@ class Lint : public InstVisitor<Lint> {
void visitReturnInst(ReturnInst &I);
void visitLoadInst(LoadInst &I);
void visitStoreInst(StoreInst &I);
void visitAtomicCmpXchgInst(AtomicCmpXchgInst &I);
void visitAtomicRMWInst(AtomicRMWInst &I);
void visitXor(BinaryOperator &I);
void visitSub(BinaryOperator &I);
void visitLShr(BinaryOperator &I);
Expand All @@ -124,6 +127,7 @@ class Lint : public InstVisitor<Lint> {

public:
Module *Mod;
Triple TT;
const DataLayout *DL;
AliasAnalysis *AA;
AssumptionCache *AC;
Expand All @@ -135,8 +139,8 @@ class Lint : public InstVisitor<Lint> {

Lint(Module *Mod, const DataLayout *DL, AliasAnalysis *AA,
AssumptionCache *AC, DominatorTree *DT, TargetLibraryInfo *TLI)
: Mod(Mod), DL(DL), AA(AA), AC(AC), DT(DT), TLI(TLI),
MessagesStr(Messages) {}
: Mod(Mod), TT(Triple::normalize(Mod->getTargetTriple())), DL(DL), AA(AA),
AC(AC), DT(DT), TLI(TLI), MessagesStr(Messages) {}

void WriteValues(ArrayRef<const Value *> Vs) {
for (const Value *V : Vs) {
Expand Down Expand Up @@ -401,6 +405,11 @@ void Lint::visitMemoryReference(Instruction &I, const MemoryLocation &Loc,
"Unusual: Address one pointer dereference", &I);

if (Flags & MemRef::Write) {
if (TT.isAMDGPU())
Check(!AMDGPU::isConstantAddressSpace(
UnderlyingObject->getType()->getPointerAddressSpace()),
"Undefined behavior: Write to memory in const addrspace", &I);

if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(UnderlyingObject))
Check(!GV->isConstant(), "Undefined behavior: Write to read-only memory",
&I);
Expand Down Expand Up @@ -480,6 +489,16 @@ void Lint::visitStoreInst(StoreInst &I) {
I.getOperand(0)->getType(), MemRef::Write);
}

void Lint::visitAtomicCmpXchgInst(AtomicCmpXchgInst &I) {
visitMemoryReference(I, MemoryLocation::get(&I), I.getAlign(),
I.getOperand(0)->getType(), MemRef::Write);
}

void Lint::visitAtomicRMWInst(AtomicRMWInst &I) {
visitMemoryReference(I, MemoryLocation::get(&I), I.getAlign(),
I.getOperand(0)->getType(), MemRef::Write);
}

void Lint::visitXor(BinaryOperator &I) {
Check(!isa<UndefValue>(I.getOperand(0)) || !isa<UndefValue>(I.getOperand(1)),
"Undefined result: xor(undef, undef)", &I);
Expand Down
49 changes: 49 additions & 0 deletions llvm/test/Analysis/Lint/const-store.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
; RUN: not opt --mtriple=amdgcn --passes=lint --lint-abort-on-error %s -disable-output 2>&1 | FileCheck %s
; RUN: opt --mtriple=amdgcn --mcpu=gfx1030 --passes=lint %s -disable-output 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK0
; RUN: opt --mtriple=x86_64 --passes=lint --lint-abort-on-error %s -disable-output 2>&1 | FileCheck %s --allow-empty --check-prefix=NOERR
; NOERR: {{^$}}

define amdgpu_kernel void @store_const(ptr addrspace(4) %out, i32 %a, i32 %b) {
; CHECK: Undefined behavior: Write to memory in const addrspace
; CHECK-NEXT: store i32 %r, ptr addrspace(4) %out
%r = add i32 %a, %b
store i32 %r, ptr addrspace(4) %out
ret void
}

declare void @llvm.memset.p4.i64(ptr addrspace(4) noalias nocapture writeonly, i8, i64, i1)
define amdgpu_kernel void @memset_const(ptr addrspace(4) %dst) {
; CHECK0: Undefined behavior: Write to memory in const addrspace
; CHECK0-NEXT: call void @llvm.memset.p4.i64(ptr addrspace(4) %dst, i8 0, i64 256, i1 false)
call void @llvm.memset.p4.i64(ptr addrspace(4) %dst, i8 0, i64 256, i1 false)
ret void
}

declare void @llvm.memcpy.p6.p0.i32(ptr addrspace(6) noalias nocapture writeonly, ptr noalias nocapture readonly, i32, i1)
define amdgpu_kernel void @memcpy_to_const(ptr addrspace(6) %dst, ptr %src) {
; CHECK0: Undefined behavior: Write to memory in const addrspace
; CHECK0-NEXT: call void @llvm.memcpy.p6.p0.i32(ptr addrspace(6) %dst, ptr %src, i32 256, i1 false)
call void @llvm.memcpy.p6.p0.i32(ptr addrspace(6) %dst, ptr %src, i32 256, i1 false)
ret void
}

define amdgpu_kernel void @cmpxchg_to_const(ptr addrspace(4) %dst, i32 %src) {
; CHECK0: Undefined behavior: Write to memory in const addrspace
; CHECK0-NEXT: %void = cmpxchg ptr addrspace(4) %dst, i32 0, i32 %src seq_cst monotonic
%void = cmpxchg ptr addrspace(4) %dst, i32 0, i32 %src seq_cst monotonic
ret void
}

define amdgpu_kernel void @atomicrmw_to_const(ptr addrspace(4) %dst, i32 %src) {
; CHECK0: Undefined behavior: Write to memory in const addrspace
; CHECK0-NEXT: %void = atomicrmw add ptr addrspace(4) %dst, i32 %src acquire
%void = atomicrmw add ptr addrspace(4) %dst, i32 %src acquire
ret void
}
Copy link
Contributor

@arsenm arsenm Sep 24, 2024

Choose a reason for hiding this comment

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

Maybe test arbitrary call that may write (those probably should not fail )


declare void @const_param(ptr addrspace(6))
define amdgpu_kernel void @call_with_const(ptr addrspace(6) %dst) {
; CHECK0-NOT: call void @const_param(ptr addrspace(6) %dst)
call void @const_param(ptr addrspace(6) %dst)
ret void
}