[AMDGPU] Alloca is not properly promoted

[shiltian]

To reproduce this issue, both LLVM and OpenMP (with target offloading at least for AMDGPU) need to be built.

The reproducer is in LLVM repo.

$ clang -fopenmp --offload-arch=gfx90a llvm-project/openmp/libomptarget/test/offloading/bug51982.c -O1 -fopenmp-target-jit -o bug51982
$ ./bug51982

At this moment the test should not fail. Extract the attachment (as-puzzle.tar.gz) and there are four files: good.ll, good.s, bad.ll, and bad.s. As their names suggest, "good" means the IR can pass the test, while "bad" means the IR causes the test failure. The only difference between the two IRs is, the bad version has an extra AS cast, and then the corresponding store to the original generic pointer is replaced with a store with the right AS. The two .s files correspond to the IRs.

In order to reproduce the runtime memory access fault, we don't need to rebuilt the reproducer.

$ llc -O1 -march=amdgcn -mcpu=gfx90a --filetype=obj good.ll -o good.o
$ export LIBOMPTARGET_JIT_REPLACEMENT_OBJECT="good.o"
$ ./bug51982

The test should pass as well. Now we compile the bad version.

$ llc -O1 -march=amdgcn -mcpu=gfx90a --filetype=obj bad.ll -o bad.o
$ export LIBOMPTARGET_JIT_REPLACEMENT_OBJECT="bad.o"
$ ./bug51982

Now you are supposed to see the memory access fault error that crashes the program.

[llvmbot]

@llvm/issue-subscribers-backend-amdgpu

To reproduce this issue, both LLVM and OpenMP (with target offloading at least for AMDGPU) need to be built.

The reproducer is in LLVM repo.

$ clang -fopenmp --offload-arch=gfx90a llvm-project/openmp/libomptarget/test/offloading/bug51982.c -O1 -fopenmp-target-jit -o bug51982
$ ./bug51982

At this moment the test should not fail. Extract the attachment. We can see there are four files: good.ll, good.s, bad.ll, and bad.s. As their name suggests, "good" means the IR can pass the test, while "bad" means the IR causes the test failure. The only difference between the two IRs is, the bad version has an extra AS cast, and then the corresponding store to the original generic pointer is replaced with a store with the right AS. The two .s files correspond to the IRs.

In order to reproduce the runtime memory access fault, we don't need to rebuilt the reproducer.

$ llc -O1 -march=amdgcn -mcpu=gfx90a --filetype=obj good.ll -o good.o
$ export LIBOMPTARGET_JIT_REPLACEMENT_OBJECT="good.o"
$ ./bug51982

The test should pass as well. Now we compile the bad version.

$ llc -O1 -march=amdgcn -mcpu=gfx90a --filetype=obj bad.ll -o bad.o
$ export LIBOMPTARGET_JIT_REPLACEMENT_OBJECT="bad.o"
$ ./bug51982

Now you are supposed to see the memory access fault error that crashes the program.

[shiltian]

ping

[Pierre-vh]

I looked at logs and ISA diff and the only difference is that we use a buffer_store in the bad case (and there is a null pointer check).

It looks like before calling, the pointers are all casted to the generic AS, and then it crashes when casted back to the private AS (5) and we emit a buffer_store. When not casted back, we use a flat_store.
Note that there are other stores already using a flat_store. Bad code is on the left, good one on the right:

Where does the code come from? Did the frontend emit the additional addrspacecast? I'm not convinced the compiler is doing something wrong yet.

Maybe @jayfoad or @nhaehnle have other ideas?

[jayfoad]

Bad code is on the right, good one on the left:

Are you sure? bad.ll stores to addrspace(5) which should generate the buffer_store (using the scratch descriptor).

Anyway I don't see any evidence that the backend is doing anything wrong.

[Pierre-vh]

Bad code is on the right, good one on the left:

Are you sure? bad.ll stores to addrspace(5) which should generate the buffer_store (using the scratch descriptor).

Anyway I don't see any evidence that the backend is doing anything wrong.

Ah yeah my bad, bad.s is on the left, good.s is on the right.

[shiltian]

I have a middle end patch (which has been reverted due to this issue) that can deduce AS and add corresponding cast to pointer operand of store and load instruction. As the two IRs show here, the only difference here is just the AS cast of one pointer, which is indeed in that AS. With this AS, a memory access fault occurs at runtime.

[jayfoad]

This looks like a bug in the AMDGPUPromoteAlloca pass. First your pass runs, which adds code in __kmpc_for_static_init_4u that "knows" that this pointer is really in addrspace(5). Then AMDGPUPromoteAlloca "promotes" the original alloca (addrspace 5) to a global variable (addrspace 3). It really should not do this unless it can prove that all accesses to the pointer can be safely updated.

[shiltian]

The root cause is we promote those allocas to LDS.

Here is a small reproducer:

define internal void @callee(ptr nocapture %0) {
  %2 = addrspacecast ptr %0 to ptr addrspace(5)
  store i32 10, ptr addrspace(5) %2, align 4
  ret void
}

define protected amdgpu_kernel void @kernel() {
  %1 = alloca i32, align 4, addrspace(5)
  %2 = addrspacecast ptr addrspace(5) %1 to ptr
  call void @callee(ptr %2)
  ret void
}

Run the following command:

$ llc -O1 -mtriple=amdgcn-amd-amdhsa -mcpu=gfx1030 small.ll -o -

We can see the alloca is promoted to LDS:

...
        s_mov_b64 s[4:5], src_shared_base
        v_mov_b32_e32 v1, s5
        v_add_lshl_u32 v0, v0, v2, 2
        s_swappc_b64 s[30:31], s[6:7]
...

Here are two questions:

1. Does the use of AS cast in the callee violate the nocapture function argument attribute? Can AS cast capture? Is it target dependent?
2. When we check all uses of the alloca, is it 100% safe by just checking whether the pointer is captured? Does nocapture mean we can safely move the object into other AS? Apparently if AS cast is considered as nocapture, it is not safe to assume that.

@jayfoad @arsenm @changpeng @jdoerfert @rampitec @Pierre-vh

[arsenm]

The call use is supposed to count as an address capture which we check for. Really we want a slightly different form of nocapture. Ideally we could also do the promotion interprocedurally. I think the easiest solution is to add an ignore nocapture to the maybecaptured call

[shiltian]

The call use is supposed to count as an address capture which we check for.

Does this mean AS cast should be counted as capture? If so, the IR is UB because the argument attribute says no capture but in reality it is.

[arsenm]

Does this mean AS cast should be counted as capture? If so, the IR is UB because the argument attribute says no capture but in reality it is.

No. We want something that's almost nocapture, but doesn't exist. Ideally we would have some similar attribute about whether the address space is significant. Failing that, I think the is-captured utility function could have a parameter to ignore no capture callsites

[jayfoad]

"Capturing" seems to be entirely the wrong concept here. Why do we use PointerMayBeCaptured in the AddrSpaceCast case, instead of using the same worklist processing that we use for everything else?