[Verifier] Ban GEP, load, store of addrspace(8) on AMDGPU

[krzysz00]

Summary

AMDGPU buffer resources (address space 8) have a complex
representation (they're 80 bits of metadata followed by 48 bits of
address) and a complex set of addressing modes (such as structured
buffer addressing, which separates out an "index" and "offset" and
allow for things like in-hardware swizzling). While these resources
are represented as pointers to enable alias analysis and other
pointer-based reasoning, they must not be manipulated using standard
LLVM instructions.

This commit adds the cases to the LLVM verifier needed to prohibit
such usage.

Depends on D146761

[foad]

llvm/lib/IR/Verifier.cpp

3875

It feels wrong to me to have target-dependent checks in the IR verifier, but I don't know if it is actually prohibited.

TT crept into the verifier in D65686 as a way to implement object-file-format-dependent checks.

[arsenm]

We don't have any real target specific verifier checks like this now. I do think we need some kind of target IR verifier, but just dropping this in like this is probably not the way to go about it

llvm/lib/IR/Verifier.cpp

3876–3878

Can use GEP.getAddressSpace().

This also will miss constant expressions

[krzysz00]

I wasn't sure if the general IR verifier is the absolute best place, but it already has calling-convention specific checks, which, at least in our case, are target specific.

[arsenm]

In D146762#4236828, @krzysz00 wrote:

I wasn't sure if the general IR verifier is the absolute best place, but it already has calling-convention specific checks, which, at least in our case, are target specific.

Calling conventions aren't quite the same, even if you always only ever use the calling convention on a specific target in the real world. It's not changing the semantics of a core IR construct based on a target check

[krzysz00]

This is my proposed stopgap measure to stop people from tripping over the target -specitic rule we'd need that you can't take offsets of buffer resources except through the arguments to our intrinsics.

Doing this in a less hacky way is probably equivalent to @nhaehnle 's proposal for structured GEP, where we'd gain a data layout flag for "these pointers have inherently complex addressing modes, you can't GEP them correctly".

llvm/lib/IR/Verifier.cpp

if (auto *GEPVTy = dyn_cast<VectorType>(GEP.getType())) {
     }
   }
 
   if (auto *PTy = dyn_cast<PointerType>(GEP.getType())) {
     Check(GEP.getAddressSpace() == PTy->getAddressSpace(),
           "GEP address space doesn't match type", &GEP);
   }
 
+  if (TT.isAMDGCN()) {
+    auto *PTy = dyn_cast<PointerType>(GEP.getPointerOperandType());
+    if (auto *VPtrTy = dyn_cast<VectorType>(GEP.getPointerOperandType()))
+      PTy = dyn_cast<PointerType>(VPtrTy->getElementType());
+    Check(!PTy || PTy->getAddressSpace() != 8,
+          "AMDGCN does not allow GEP of buffer resources (addrspace 8)", &GEP);
+  }
+
   visitInstruction(GEP);
 }
 
 static bool isContiguous(const ConstantRange &A, const ConstantRange &B) {
   return A.getUpper() == B.getLower() || A.getLower() == B.getUpper();
 }
 
 void Verifier::visitRangeMetadata(Instruction &I, MDNode *Range, Type *Ty) {
Check(ElTy->isIntOrPtrTy() || ElTy->isFloatingPointTy(),
           "type!",
           ElTy, &LI);
     checkAtomicMemAccessSize(ElTy, &LI);
   } else {
     Check(LI.getSyncScopeID() == SyncScope::System,
           "Non-atomic load cannot have SynchronizationScope specified", &LI);
   }
 
+  if (TT.isAMDGCN()) {
+    Check(PTy->getAddressSpace() != 8,
+          "AMDGCN does not allow loads from buffer resources (addrspace 8), "
+          "use intrinsics instead",
+          &LI);
+  }
+
   visitInstruction(LI);
 }
 
 void Verifier::visitStoreInst(StoreInst &SI) {
   PointerType *PTy = dyn_cast<PointerType>(SI.getOperand(1)->getType());
   Check(PTy, "Store operand must be a pointer.", &SI);
   Type *ElTy = SI.getOperand(0)->getType();
   Check(PTy->isOpaqueOrPointeeTypeMatches(ElTy),
Check(ElTy->isIntOrPtrTy() || ElTy->isFloatingPointTy(),
           "atomic store operand must have integer, pointer, or floating point "
           "type!",
           ElTy, &SI);
     checkAtomicMemAccessSize(ElTy, &SI);
   } else {
     Check(SI.getSyncScopeID() == SyncScope::System,
           "Non-atomic store cannot have SynchronizationScope specified", &SI);
   }
+
+  if (TT.isAMDGCN()) {
+    Check(PTy->getAddressSpace() != 8,
+          "AMDGCN does not allow stores to buffer resources (addrspace 8), use "
+          "intrinsics instead",
+          &SI);
+  }
+
   visitInstruction(SI);
 }
 
 /// Check that SwiftErrorVal is used as a swifterror argument in CS.
 void Verifier::verifySwiftErrorCall(CallBase &Call,
                                     const Value *SwiftErrorVal) {
   for (const auto &I : llvm::enumerate(Call.args())) {
     if (I.value() == SwiftErrorVal) {

llvm/test/CodeGen/AMDGPU/GlobalISel/unsupported-load.ll

-; RUN: not --crash llc -global-isel=1 -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -o - < %s 2>&1 | FileCheck -check-prefix=GISEL-ERR %s
+; RUN: not --crash llc -disable-verify -global-isel=1 -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -o - < %s 2>&1 | FileCheck -check-prefix=GISEL-ERR %s
 
 ; GISEL-ERR: LLVM ERROR: cannot select: %{{[0-9]+}}:vgpr_32(s32) = G_LOAD %{{[0-9]+}}:vgpr(p8) :: (load (s32) from %ir.rsrc, addrspace 8)
 
 define float @load_from_rsrc(ptr addrspace(8) %rsrc) {
 body:
   %res = load float, ptr addrspace(8) %rsrc
   ret float %res
 }

llvm/test/CodeGen/AMDGPU/GlobalISel/unsupported-ptr-add.ll

-; RUN: not --crash llc -global-isel=1 -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -o - < %s 2>&1 | FileCheck -check-prefix=GISEL-ERR %s
+; RUN: not --crash llc -disable-verify -global-isel=1 -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -o - < %s 2>&1 | FileCheck -check-prefix=GISEL-ERR %s
 
 ; GISEL-ERR: LLVM ERROR: unable to legalize instruction: %{{[0-9]+}}:_(p8) = G_PTR_ADD %{{[0-9]+}}:_, %{{[0-9]+}}:_(s128)
 
 
 define float @gep_on_rsrc(ptr addrspace(8) %rsrc) {
 body:
   %next = getelementptr float, ptr addrspace(8) %rsrc, i128 1
   %res = call float @llvm.amdgcn.raw.buffer.load.f32(ptr addrspace(8) %next, i32 0, i32 0, i32 0)
   ret float %res
 }
 
 declare float @llvm.amdgcn.raw.buffer.load.f32(ptr addrspace(8), i32, i32, i32 immarg)
 

llvm/test/Verifier/AMDGPU/buffer-rsrc-pointers.ll

+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
+
+target triple = "amdgcn--";
+
+define ptr addrspace(8) @scalar_gep(ptr addrspace(8) %p) {
+  ; CHECK: AMDGCN does not allow GEP of buffer resources (addrspace 8)
+  ; CHECK-NEXT: %ret = getelementptr i8, ptr addrspace(8) %p, i32 0
+  %ret = getelementptr i8, ptr addrspace(8) %p, i32 0
+  ret ptr addrspace(8) %ret
+}
+
+define <2 x ptr addrspace(8)> @vec_gep(<2 x ptr addrspace(8)> %p) {
+  ; CHECK: AMDGCN does not allow GEP of buffer resources (addrspace 8)
+  ; CHECK-NEXT: %ret = getelementptr i8, <2 x ptr addrspace(8)> %p, <2 x i32> <i32 0, i32 1>
+  %ret = getelementptr i8, <2 x ptr addrspace(8)> %p, <2 x i32> <i32 0, i32 1>
+  ret <2 x ptr addrspace(8)> %ret
+}
+
+define i32 @load(ptr addrspace(8) %p) {
+  ; CHECK: AMDGCN does not allow loads from buffer resources (addrspace 8), use intrinsics instead
+  ; CHECK-NEXT: %ret = load i32, ptr addrspace(8) %p, align 4
+  %ret = load i32, ptr addrspace(8) %p
+  ret i32 %ret
+}
+
+define void @store(ptr addrspace(8) %p) {
+  ; CHECK: AMDGCN does not allow stores to buffer resources (addrspace 8), use intrinsics instead
+  ; CHECK-NEXT: store i32 0, ptr addrspace(8) %p, align 4
+  store i32 0, ptr addrspace(8) %p
+  ret void
+}