AMDGPU: Don't use unreachable on stores to unhandled address space

[arsenm]

Summary

For stores to constant address space, this will now consistently hit a
selection error instead of hitting unreachable in an asserts build.

I'm not sure what we should really do here. We could either just
codegen as if it were global, delete the instruction, or declare the
IR invalid (we really should have a target IR verifier to enforce it).

[rampitec]

In fact not only the stores but atomics as well including atomic load. It is probably better to declare it an invalid IR. The only exception when it might make any sense to lower it as a global is the atomic load, although it is not really needed so there is no need to open this can of worms.

[arsenm]

In D120286#3336509, @rampitec wrote:

In fact not only the stores but atomics as well including atomic load. It is probably better to declare it an invalid IR. The only exception when it might make any sense to lower it as a global is the atomic load, although it is not really needed so there is no need to open this can of worms.

We at least need to produce the same error in debug vs. release, which this achieves

[rampitec]

LGTM anyway. I believe atomics need it too.

[arsenm]

788f94f731dc9ba514925ed0c5936c5e79f6abd8

[dyung]

Hi @arsenm, the test you added is failing on our internal linux build bot that builds a release build without asserts with the following error:

Command Output (stderr):
--
/home/dyung/src/upstream/llvm_clean_git/llvm/test/CodeGen/AMDGPU/store-to-constant-error.ll:4:9: error: SDAG: expected string not found in input
; SDAG: LLVM ERROR: Cannot select: t{{[0-9]+}}: ch = store<(store (s32) into %ir.ptr.load, addrspace 4)>
        ^
<stdin>:1:1: note: scanning from here
LLVM ERROR: Cannot select: 0x55f05ad6f228: ch = store<(store (s32) into %ir.ptr.load, addrspace 4)> 0x55f05acd7d38, Constant:i32<1>, 0x55f05ad6f020, undef:i64
^

Can you take a look?

[arsenm]

In D120286#3449116, @dyung wrote:

Hi @arsenm, the test you added is failing on our internal linux build bot that builds a release build without asserts with the following error:

Command Output (stderr):
--
/home/dyung/src/upstream/llvm_clean_git/llvm/test/CodeGen/AMDGPU/store-to-constant-error.ll:4:9: error: SDAG: expected string not found in input
; SDAG: LLVM ERROR: Cannot select: t{{[0-9]+}}: ch = store<(store (s32) into %ir.ptr.load, addrspace 4)>
        ^
<stdin>:1:1: note: scanning from here
LLVM ERROR: Cannot select: 0x55f05ad6f228: ch = store<(store (s32) into %ir.ptr.load, addrspace 4)> 0x55f05acd7d38, Constant:i32<1>, 0x55f05ad6f020, undef:i64
^

Can you take a look?

This is because the nodes in the debug output change from numbered notes to raw pointers in release builds

[foad]

In D120286#3449357, @arsenm wrote:

In D120286#3449116, @dyung wrote:

Hi @arsenm, the test you added is failing on our internal linux build bot that builds a release build without asserts with the following error:

Command Output (stderr):
--
/home/dyung/src/upstream/llvm_clean_git/llvm/test/CodeGen/AMDGPU/store-to-constant-error.ll:4:9: error: SDAG: expected string not found in input
; SDAG: LLVM ERROR: Cannot select: t{{[0-9]+}}: ch = store<(store (s32) into %ir.ptr.load, addrspace 4)>
        ^
<stdin>:1:1: note: scanning from here
LLVM ERROR: Cannot select: 0x55f05ad6f228: ch = store<(store (s32) into %ir.ptr.load, addrspace 4)> 0x55f05acd7d38, Constant:i32<1>, 0x55f05ad6f020, undef:i64
^

Can you take a look?

This is because the nodes in the debug output change from numbered notes to raw pointers in release builds

For the record this was fixed by e78f70bccb8919d74bbbe492aa2fb0eb230b7467.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

if (AS == AMDGPUAS::GLOBAL_ADDRESS ||
     if (!allowsMemoryAccessForAlignment(*DAG.getContext(), DAG.getDataLayout(),
                                         VT, *Store->getMemOperand())) {
       if (VT.isVector())
         return SplitVectorStore(Op, DAG);
       return expandUnalignedStore(Store, DAG);
     }
 
     return SDValue();
-  } else {
-    llvm_unreachable("unhandled address space");
   }
+
+  // Probably an invalid store. If so we'll end up emitting a selection error.
+  return SDValue();
 }
 
 SDValue SITargetLowering::LowerTrig(SDValue Op, SelectionDAG &DAG) const {
   SDLoc DL(Op);
   EVT VT = Op.getValueType();
   SDValue Arg = Op.getOperand(0);
   SDValue TrigVal;
 

llvm/test/CodeGen/AMDGPU/store-to-constant-error.ll

+; RUN: not --crash llc -global-isel=0 -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -o /dev/null %s 2>&1 | FileCheck -check-prefix=SDAG %s
+; RUN: not --crash llc -global-isel=1 -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -o /dev/null %s 2>&1 | FileCheck -check-prefix=GISEL %s
+
+; SDAG: LLVM ERROR: Cannot select: t{{[0-9]+}}: ch = store<(store (s32) into %ir.ptr.load, addrspace 4)>
+; GISEL: LLVM ERROR: cannot select: G_STORE %{{[0-9]+}}:vgpr(s32), %{{[0-9]+}}:vgpr(p4) :: (store (s32) into %ir.ptr.load, addrspace 4) (in function: store_to_constant_i32)
+define amdgpu_kernel void @store_to_constant_i32(i32 addrspace(4)* %ptr) {
+bb:
+  store i32 1, i32 addrspace(4)* %ptr, align 4
+  ret void
+}