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

jofrn · 2024-09-18T19:40:41Z

Ensure store to const addrspace is not allowed by IR Verifier.

llvmbot · 2024-09-18T19:41:14Z

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-amdgpu

Author: None (jofrn)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/109181.diff

2 Files Affected:

(modified) llvm/lib/IR/Verifier.cpp (+17)
(added) llvm/test/CodeGen/AMDGPU/const-store.ll (+9)

diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 06a67346fbf959..805c9ef7b72729 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -4224,6 +4224,19 @@ void Verifier::visitLoadInst(LoadInst &LI) {
   visitInstruction(LI);
 }
 
+static bool isConstantAddressSpace(unsigned AS) {
+  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);
@@ -4246,6 +4259,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);
 }
 
diff --git a/llvm/test/CodeGen/AMDGPU/const-store.ll b/llvm/test/CodeGen/AMDGPU/const-store.ll
new file mode 100644
index 00000000000000..9447ef9db59986
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/const-store.ll
@@ -0,0 +1,9 @@
+; RUN: not llc -mtriple=amdgcn < %s |& FileCheck %s
+
+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
+}

github-actions · 2024-09-18T19:44:12Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 5f02558d820fdc9aa8ac7d3d887e72526574e1d9 393311ca3330fd9ed7baa598e032fc8ee1ebaa66 --extensions cpp,h -- llvm/include/llvm/Support/AMDGPUAddrSpace.h llvm/lib/Analysis/Lint.cpp

View the diff from clang-format here.

diff --git a/llvm/include/llvm/Support/AMDGPUAddrSpace.h b/llvm/include/llvm/Support/AMDGPUAddrSpace.h
index 6f03c3fca3..a7533b99a8 100644
--- a/llvm/include/llvm/Support/AMDGPUAddrSpace.h
+++ b/llvm/include/llvm/Support/AMDGPUAddrSpace.h
@@ -95,7 +95,7 @@ inline bool isExtendedGlobalAddrSpace(unsigned AS) {
 }
 
 inline bool isConstantAddressSpace(unsigned AS) {
-  switch(AS) {
+  switch (AS) {
     using namespace AMDGPUAS;
   case CONSTANT_ADDRESS:
   case CONSTANT_ADDRESS_32BIT:
diff --git a/llvm/lib/Analysis/Lint.cpp b/llvm/lib/Analysis/Lint.cpp
index 3503b6980c..d76e77cd0e 100644
--- a/llvm/lib/Analysis/Lint.cpp
+++ b/llvm/lib/Analysis/Lint.cpp
@@ -122,6 +122,7 @@ class Lint : public InstVisitor<Lint> {
   Value *findValue(Value *V, bool OffsetOk) const;
   Value *findValueImpl(Value *V, bool OffsetOk,
                        SmallPtrSetImpl<Value *> &Visited) const;
+
 public:
   Module *Mod;
   Triple TT;
@@ -136,9 +137,8 @@ public:
 
   Lint(Module *Mod, const DataLayout *DL, AliasAnalysis *AA,
        AssumptionCache *AC, DominatorTree *DT, TargetLibraryInfo *TLI)
-      : Mod(Mod), TT(Triple::normalize(Mod->getTargetTriple())),
-	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) {
@@ -404,8 +404,9 @@ void Lint::visitMemoryReference(Instruction &I, const MemoryLocation &Loc,
 
   if (Flags & MemRef::Write) {
     if (TT.isAMDGPU())
-      Check(!AMDGPU::isConstantAddressSpace(UnderlyingObject->getType()->getPointerAddressSpace()),
-        "Undefined behavior: Write to const memory", &I);
+      Check(!AMDGPU::isConstantAddressSpace(
+                UnderlyingObject->getType()->getPointerAddressSpace()),
+            "Undefined behavior: Write to const memory", &I);
 
     if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(UnderlyingObject))
       Check(!GV->isConstant(), "Undefined behavior: Write to read-only memory",

Ensure store to const addrspace is not allowed by IR Verifier.

llvm/test/CodeGen/AMDGPU/const-store.ll

shiltian · 2024-09-19T04:27:40Z

llvm/lib/IR/Verifier.cpp

@@ -4224,6 +4224,33 @@ void Verifier::visitLoadInst(LoadInst &LI) {
  visitInstruction(LI);
 }

+static bool isConstantAddressSpace(unsigned AS) {


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.

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

jayfoad · 2024-09-19T09:51:28Z

Was there any discussion about this? I'm not sure if it should be a verifier error or just undefined behaviour at run time. Also there are other instructions that write to memory, like atomicrmw.

arsenm

This must be UB, and not a verifier error. For example, we allow constant->flat casts. We do not want to have to consider the address spaces involved when folding something like store (addrspacecast ptr addrspace(4) %val to ptr)

arsenm · 2024-09-20T04:21:26Z

Plus long term I don't think we should have a constant address space at all.

jofrn · 2024-09-20T13:13:30Z

Was there any discussion about this? I'm not sure if it should be a verifier error or just undefined behaviour at run time. Also there are other instructions that write to memory, like atomicrmw.

It can be guarded with a flag perhaps. There was discussion to catch this in some way or another since it leads to more confusing error messages later on. Alternatively, we can have these checks go in a separate pass.

jofrn · 2024-09-20T13:19:57Z

This must be UB, and not a verifier error. For example, we allow constant->flat casts. We do not want to have to consider the address spaces involved when folding something like store (addrspacecast ptr addrspace(4) %val to ptr)

The addrspace in store (addrspacecast ptr addrspace(4) %val to ptr) won't be affected by getPointerAddressSpace since this function asks about the second operand to the store, store i32 %val, ptr addrspace(4) %ptr, in particular, ptr addrspace(4) %ptr. Your example gives a complex expression on the value source, the first operand.

nikic · 2024-09-20T13:28:23Z

These kinds of checks should go into the Lint pass instead of the Verifier pass.

arsenm · 2024-09-20T13:28:32Z

The addrspace in store (addrspacecast ptr addrspace(4) %val to ptr) won't be affected by getPointerAddressSpace since this function asks about the second operand to the store, store i32 %val, ptr addrspace(4) %ptr, in particular, ptr addrspace(4) %ptr. Your example gives a complex expression on the value source, the first operand.

You're reading the example too literally. It should be valid for a pass to strip away the cast, and store directly to the original address space of the pointer without considering any special information. It would be a target specific burden placed on any IR transformation, which is not OK.

This type of check belongs in Lint, which hardly ever gets used and I'm not sure is worth the effort

arsenm · 2024-09-20T18:40:20Z

llvm/test/CodeGen/AMDGPU/const-store.ll

+  %r = add i32 %a, %b
+  store i32 %r, ptr addrspace(4) %out
+  ret void
+}


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

arsenm · 2024-09-20T18:40:36Z

llvm/test/CodeGen/AMDGPU/const-store.ll

@@ -0,0 +1,9 @@
+; RUN: not opt -mtriple=amdgcn --passes=lint --lint-abort-on-error %s -o - |& FileCheck %s
+
+define amdgpu_kernel void @store_const(ptr addrspace(4) %out, i32 %a, i32 %b) addrspace(4) {


no reason to set the address space of the function

arsenm · 2024-09-20T18:40:47Z

llvm/test/CodeGen/AMDGPU/const-store.ll

@@ -0,0 +1,9 @@
+; RUN: not opt -mtriple=amdgcn --passes=lint --lint-abort-on-error %s -o - |& FileCheck %s
+


negative test with a different target

arsenm · 2024-09-20T18:41:08Z

llvm/test/CodeGen/AMDGPU/const-store.ll

@@ -0,0 +1,9 @@
+; RUN: not opt -mtriple=amdgcn --passes=lint --lint-abort-on-error %s -o - |& FileCheck %s


Should move this to test/Analysis/Lint

nikic

New approach looks reasonable to me.

nikic · 2024-09-21T07:33:25Z

llvm/lib/Analysis/Lint.cpp

+  if (TT.isAMDGPU()) {
+    switch(AS) {
+      using namespace AMDGPUAS;
+    case CONSTANT_ADDRESS:


Should this be a helper in AMDGPUAddrSpace.h? I'd expect this logic to already exist somewhere, but it looks like AMDGPUAA is only looking for CONSTANT_ADDRESS and nothing else:

llvm-project/llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp

Line 78 in 97aa8cc

if (LI->getPointerAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS)

shiltian · 2024-09-21T15:43:41Z

llvm/lib/Analysis/Lint.cpp

@@ -401,6 +403,10 @@ 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()),


Is it possible or better to do the check via TTI? I'm not sure if it's a good idea to do the check based on triple.

shiltian · 2024-09-21T15:44:01Z

llvm/test/Analysis/Lint/const-store.ll

+; RUN: opt --mtriple=amdgcn --mcpu=gfx1030 --passes=lint %s -o /dev/null |& FileCheck %s --check-prefixes=CHECK,CHECK0
+; RUN: opt --mtriple=x86_64 --passes=lint --lint-abort-on-error %s -o /dev/null |& FileCheck %s --allow-empty --check-prefix=NOERR
+; NOERR: {{^$}}
+


I think you will need ; REQUIRES: amdgpu-registered-target as well as other targets that you want to test.

jofrn requested a review from arsenm September 18, 2024 19:40

llvmbot added backend:AMDGPU llvm:ir labels Sep 18, 2024

jofrn requested a review from shiltian September 18, 2024 19:41

jofrn force-pushed the amdgpu-irverifier branch from 7a77ede to 5469cf1 Compare September 18, 2024 19:45

[Verifier][AMDGPU] No store to const addrspace

a099028

Ensure store to const addrspace is not allowed by IR Verifier.

jofrn force-pushed the amdgpu-irverifier branch from 5469cf1 to a099028 Compare September 18, 2024 19:57

shiltian reviewed Sep 19, 2024

View reviewed changes

arsenm requested changes Sep 20, 2024

View reviewed changes

jofrn force-pushed the amdgpu-irverifier branch from a82bcdc to b3fea0b Compare September 20, 2024 18:17

llvmbot added the llvm:analysis label Sep 20, 2024

arsenm reviewed Sep 20, 2024

View reviewed changes

jofrn force-pushed the amdgpu-irverifier branch 2 times, most recently from 0b69206 to b1c3d38 Compare September 20, 2024 19:52

[Lint] Switch check to Linter

4553484

jofrn force-pushed the amdgpu-irverifier branch from b1c3d38 to 4553484 Compare September 20, 2024 19:57

jofrn changed the title ~~[Verifier][AMDGPU] No store to const addrspace~~ [Lint][AMDGPU] No store to const addrspace Sep 20, 2024

jofrn force-pushed the amdgpu-irverifier branch from 14a876d to 2c640e5 Compare September 21, 2024 06:11

nikic reviewed Sep 21, 2024

View reviewed changes

Add negative test, move isConstantAddressSpace

393311c

jofrn force-pushed the amdgpu-irverifier branch from 2c640e5 to 393311c Compare September 21, 2024 15:35

llvmbot added the llvm:support label Sep 21, 2024

shiltian reviewed Sep 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jofrn commented Sep 18, 2024 •

edited

Loading

llvmbot commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

shiltian Sep 19, 2024

jofrn Sep 19, 2024

jayfoad commented Sep 19, 2024

arsenm left a comment

arsenm commented Sep 20, 2024

jofrn commented Sep 20, 2024 •

edited

Loading

jofrn commented Sep 20, 2024 •

edited

Loading

nikic commented Sep 20, 2024

arsenm commented Sep 20, 2024

arsenm Sep 20, 2024

arsenm Sep 20, 2024

arsenm Sep 20, 2024

arsenm Sep 20, 2024

nikic left a comment

nikic Sep 21, 2024

shiltian Sep 21, 2024

shiltian Sep 21, 2024 •

edited

Loading

		@@ -0,0 +1,9 @@
		; RUN: not opt -mtriple=amdgcn --passes=lint --lint-abort-on-error %s -o - \|& FileCheck %s

		define amdgpu_kernel void @store_const(ptr addrspace(4) %out, i32 %a, i32 %b) addrspace(4) {

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

Are you sure you want to change the base?

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

Conversation

jofrn commented Sep 18, 2024 • edited Loading

llvmbot commented Sep 18, 2024 • edited Loading

github-actions bot commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayfoad commented Sep 19, 2024

arsenm left a comment

Choose a reason for hiding this comment

arsenm commented Sep 20, 2024

jofrn commented Sep 20, 2024 • edited Loading

jofrn commented Sep 20, 2024 • edited Loading

nikic commented Sep 20, 2024

arsenm commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiltian Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

jofrn commented Sep 18, 2024 •

edited

Loading

llvmbot commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

jofrn commented Sep 20, 2024 •

edited

Loading

jofrn commented Sep 20, 2024 •

edited

Loading

shiltian Sep 21, 2024 •

edited

Loading