[Coroutines] Change llvm.coro.noop to accept llvm_anyptr_ty instead

[jhuber6]

Summary:
These intrinsics return pointers to global values, which currently does not respect the target's default address space. This patch does the first step by modifying the llvm.coro.noop intrinsic to accept different address spaces. Porting the others should go into different patches.

Fixes: #102095

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-coroutines

@llvm/pr-subscribers-llvm-transforms

Author: Joseph Huber (jhuber6)

Changes

Summary:
This pass tries to get a pointer from the global. Some targets, like
AMDGPU, emit globals to a qualified address space. This would result in
an invalid cast. To fix this, we simply apply an addrspace cast, which
is a no-op if none exit.

Fixes: #102095

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Coroutines/CoroEarly.cpp (+4-2)
• (modified) llvm/test/Transforms/Coroutines/coro-noop.ll (+4-1)

diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
index d8e827e9cebcf..53774fd5a35d5 100644
--- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
@@ -121,8 +121,8 @@ static void buildDebugInfoForNoopResumeDestroyFunc(Function *NoopFn) {
 }
 
 void Lowerer::lowerCoroNoop(IntrinsicInst *II) {
+  LLVMContext &C = Builder.getContext();
   if (!NoopCoro) {
-    LLVMContext &C = Builder.getContext();
     Module &M = *II->getModule();
 
     // Create a noop.frame struct type.
@@ -152,7 +152,9 @@ void Lowerer::lowerCoroNoop(IntrinsicInst *II) {
   }
 
   Builder.SetInsertPoint(II);
-  auto *NoopCoroVoidPtr = Builder.CreateBitCast(NoopCoro, Int8Ptr);
+  auto *NoopCoroVoidPtr = Builder.CreateBitCast(
+      Builder.CreateAddrSpaceCast(NoopCoro, PointerType::getUnqual(C)),
+      PointerType::getUnqual(C));
   II->replaceAllUsesWith(NoopCoroVoidPtr);
   II->eraseFromParent();
 }
diff --git a/llvm/test/Transforms/Coroutines/coro-noop.ll b/llvm/test/Transforms/Coroutines/coro-noop.ll
index 1e4f19a2ef66e..cc2aa1d974355 100644
--- a/llvm/test/Transforms/Coroutines/coro-noop.ll
+++ b/llvm/test/Transforms/Coroutines/coro-noop.ll
@@ -1,8 +1,10 @@
 ; Tests that CoroEarly pass correctly lowers coro.noop
-; RUN: opt < %s -S -passes=coro-early | FileCheck %s
+; RUN: opt -S -passes=coro-early < %s | FileCheck %s
+; RUN: opt -mtriple=amdgcn-- -S -passes=coro-early < %s | FileCheck %s --check-prefix=AMDGPU
 
 ; CHECK: %NoopCoro.Frame = type { ptr, ptr }
 ; CHECK: @NoopCoro.Frame.Const = private constant %NoopCoro.Frame { ptr @__NoopCoro_ResumeDestroy, ptr @__NoopCoro_ResumeDestroy }
+; AMDGPU: @NoopCoro.Frame.Const = private addrspace(1) constant %NoopCoro.Frame { ptr @__NoopCoro_ResumeDestroy, ptr @__NoopCoro_ResumeDestroy }
 
 
 ; CHECK-LABEL: @noop(
@@ -10,6 +12,7 @@ define ptr @noop() {
 ; CHECK-NEXT: entry
 entry:
 ; CHECK-NEXT: ret ptr @NoopCoro.Frame.Const
+; AMDGPU: ret ptr addrspacecast (ptr addrspace(1) @NoopCoro.Frame.Const to ptr)
   %n = call ptr @llvm.coro.noop()
   ret ptr %n
 }

[jrtc27]

I imagine the better fix would be to either overload the intrinsic or make it return a pointer in the global address space, depending on whether its semantics are tied to globals or this is just an implementation choice.

[arsenm]

You can't insert any addrspacecast you want. Really these intrinsics should be fixed to have an address space mangled pointer. We should not have any intrinsics defined with llvm_ptr_ty

[arsenm]

Don't describe it as address space qualified. There is no such thing as a value with no address space

[jhuber6]

I imagine the better fix would be to either overload the intrinsic or make it return a pointer in the global address space, depending on whether it's semantics tied to globals or this is just an implementation choice.

It's definitely tied to globals for noop in this case, so I did what you suggested. I think we should apply this to a lot of the other ones, but they should be separate patches and this solves my immediate problem.

[arsenm]

Can you just make this a switch over the intrinsic ID instead of this weird intermediate set

[jhuber6]

I don't know how the code was originally put together, since it seems to be a subset of coroutine functions that are used here.

[arsenm]

right so switch over that set?

[jhuber6]

I thought that's what this function does, takes a subset and checks the module for any matching intrinsic IDs.

[arsenm]

But it's doing it with this intermediate set. As in, this is an overly fancy, heavy API. I mean literally loop over function declarations with a switch in it, no DenseSet