Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+28 −13
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang/lib/CodeGen/CGCoroutine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,9 @@ RValue CodeGenFunction::EmitCoroutineIntrinsic(const CallExpr *E,
if (IID == llvm::Intrinsic::coro_end)
Args.push_back(llvm::ConstantTokenNone::get(getLLVMContext()));

llvm::Function *F = CGM.getIntrinsic(IID);
llvm::Function *F = IID == llvm::Intrinsic::coro_noop
? CGM.getIntrinsic(IID, {CGM.GlobalsVoidPtrTy})
: CGM.getIntrinsic(IID);
llvm::CallInst *Call = Builder.CreateCall(F, Args);

// Note: The following code is to enable to emit coro.id and coro.begin by
Expand Down
2 changes: 1 addition & 1 deletion clang/test/CodeGenCoroutines/coro-builtins.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ void f(int n) {
// CHECK-NEXT: call i1 @llvm.coro.alloc(token %[[COROID]])
__builtin_coro_alloc();

// CHECK-NEXT: call ptr @llvm.coro.noop()
// CHECK-NEXT: call ptr @llvm.coro.noop.p0()
__builtin_coro_noop();

// CHECK-NEXT: %[[SIZE:.+]] = call i64 @llvm.coro.size.i64()
Expand Down
2 changes: 1 addition & 1 deletion llvm/include/llvm/IR/Intrinsics.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1713,7 +1713,7 @@ def int_coro_end_async
: Intrinsic<[llvm_i1_ty], [llvm_ptr_ty, llvm_i1_ty, llvm_vararg_ty], []>;

def int_coro_frame : Intrinsic<[llvm_ptr_ty], [], [IntrNoMem]>;
def int_coro_noop : Intrinsic<[llvm_ptr_ty], [], [IntrNoMem]>;
def int_coro_noop : Intrinsic<[llvm_anyptr_ty], [], [IntrNoMem]>;
def int_coro_size : Intrinsic<[llvm_anyint_ty], [], [IntrNoMem]>;
def int_coro_align : Intrinsic<[llvm_anyint_ty], [], [IntrNoMem]>;

Expand Down
15 changes: 8 additions & 7 deletions llvm/lib/Transforms/Coroutines/CoroEarly.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,7 @@ void Lowerer::lowerCoroNoop(IntrinsicInst *II) {
}

Builder.SetInsertPoint(II);
auto *NoopCoroVoidPtr = Builder.CreateBitCast(NoopCoro, Int8Ptr);
II->replaceAllUsesWith(NoopCoroVoidPtr);
II->replaceAllUsesWith(NoopCoro);
II->eraseFromParent();
}

Expand Down Expand Up @@ -249,11 +248,13 @@ void Lowerer::lowerEarlyIntrinsics(Function &F) {

static bool declaresCoroEarlyIntrinsics(const Module &M) {
return coro::declaresIntrinsics(
M, {"llvm.coro.id", "llvm.coro.id.retcon", "llvm.coro.id.retcon.once",
"llvm.coro.id.async", "llvm.coro.destroy", "llvm.coro.done",
"llvm.coro.end", "llvm.coro.end.async", "llvm.coro.noop",
"llvm.coro.free", "llvm.coro.promise", "llvm.coro.resume",
"llvm.coro.suspend"});
M, DenseSet<Intrinsic::ID>{
Intrinsic::coro_id, Intrinsic::coro_id_retcon,
Intrinsic::coro_id_retcon_once, Intrinsic::coro_id_async,
Intrinsic::coro_destroy, Intrinsic::coro_done, Intrinsic::coro_end,
Intrinsic::coro_end_async, Intrinsic::coro_noop,
Intrinsic::coro_free, Intrinsic::coro_promise,
Intrinsic::coro_resume, Intrinsic::coro_suspend});
}

PreservedAnalyses CoroEarlyPass::run(Module &M, ModuleAnalysisManager &) {
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/Transforms/Coroutines/CoroInternal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace coro {
bool declaresAnyIntrinsic(const Module &M);
bool declaresIntrinsics(const Module &M,
const std::initializer_list<StringRef>);
bool declaresIntrinsics(const Module &M, const DenseSet<llvm::Intrinsic::ID> &);
void replaceCoroFree(CoroIdInst *CoroId, bool Elide);

/// Attempts to rewrite the location operand of debug intrinsics in terms of
Expand Down
11 changes: 11 additions & 0 deletions llvm/lib/Transforms/Coroutines/Coroutines.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,17 @@ bool coro::declaresIntrinsics(const Module &M,
return false;
}

// Verifies if a module has any intrinsics.
bool coro::declaresIntrinsics(const Module &M,
const DenseSet<Intrinsic::ID> &Identifiers) {
for (const Function &F : M.functions()) {
if (Identifiers.contains(F.getIntrinsicID()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right so switch over that set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return true;
}

return false;
}

// Replace all coro.frees associated with the provided CoroId either with 'null'
// if Elide is true and with its frame parameter otherwise.
void coro::replaceCoroFree(CoroIdInst *CoroId, bool Elide) {
Expand Down
6 changes: 3 additions & 3 deletions llvm/test/Transforms/Coroutines/coro-noop.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
; 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

; CHECK: %NoopCoro.Frame = type { ptr, ptr }
; CHECK: @NoopCoro.Frame.Const = private constant %NoopCoro.Frame { ptr @__NoopCoro_ResumeDestroy, ptr @__NoopCoro_ResumeDestroy }
Expand All @@ -10,11 +10,11 @@ define ptr @noop() {
; CHECK-NEXT: entry
entry:
; CHECK-NEXT: ret ptr @NoopCoro.Frame.Const
%n = call ptr @llvm.coro.noop()
%n = call ptr @llvm.coro.noop.p1()
ret ptr %n
}

declare ptr @llvm.coro.noop()
declare ptr @llvm.coro.noop.p1()

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3, !4}
Expand Down
Loading