Author: Yuxuan Chen Date: 2026-07-22T07:39:30-07:00 New Revision: b9e3e6546dcb4b33d023b7a336163d5650cda9d4
URL: https://github.com/llvm/llvm-project/commit/b9e3e6546dcb4b33d023b7a336163d5650cda9d4 DIFF: https://github.com/llvm/llvm-project/commit/b9e3e6546dcb4b33d023b7a336163d5650cda9d4.diff LOG: [Clang][coro] Fix `coro.free` in `.resume` clones with `[[clang::coro_await_elidable]]` (#207799) Fixes https://github.com/llvm/llvm-project/issues/188230 CoroAnnotationElide rewrites annotated safe calls to the `.noalloc` variant. The noalloc frame is caller-owned, but its `.resume` clone is shared with ordinary heap-allocated instances. With a `suspend_never` final suspend, normal resumption falls through to the `coro.free` deallocation path. Regular frontend cleanup has already run before this point. `coro.free` must therefore produce the frame pointer for a heap instance and null for a `.noalloc` instance. Use the frame destroy slot as a per-instance allocation tag. Cache its value at resume entry, before user code can resume and release the enclosing caller frame, then compare it with the cleanup clone. Replace each `coro.free` result with the frame pointer for a heap instance and null for an elided instance. Update the CoroSplit and Clang CodeGen checks to cover the conditional deallocation and the original suspend_never final-suspend shape. Assisted-By: Codex GPT 5.5 Added: clang/test/CodeGenCoroutines/gh188230-coro-await-elidable-suspend-never-final.cpp llvm/test/Transforms/Coroutines/coro-split-resume-fallthrough-destroy-slot.ll Modified: llvm/include/llvm/Transforms/Coroutines/CoroShape.h llvm/lib/Transforms/Coroutines/CoroSplit.cpp llvm/lib/Transforms/Coroutines/Coroutines.cpp llvm/test/Transforms/Coroutines/coro-split-00.ll llvm/test/Transforms/Coroutines/coro-split-addrspace.ll Removed: ################################################################################ diff --git a/clang/test/CodeGenCoroutines/gh188230-coro-await-elidable-suspend-never-final.cpp b/clang/test/CodeGenCoroutines/gh188230-coro-await-elidable-suspend-never-final.cpp new file mode 100644 index 0000000000000..bcd2cff989620 --- /dev/null +++ b/clang/test/CodeGenCoroutines/gh188230-coro-await-elidable-suspend-never-final.cpp @@ -0,0 +1,79 @@ +// Tests that a coro_await_elidable coroutine with a suspend_never final suspend +// suppresses only the deallocation for a resumed elided callee. +// +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -O2 \ +// RUN: -mllvm -coro-elide-branch-ratio=0 -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/coroutine.h" + +struct gate { + std::coroutine_handle<> waiter = nullptr; + bool open = false; + + struct awaiter { + gate &g; + bool await_ready() noexcept { return g.open; } + void await_suspend(std::coroutine_handle<> h) noexcept { g.waiter = h; } + void await_resume() noexcept {} + }; + + awaiter operator co_await() noexcept { return {*this}; } +}; + +struct [[clang::coro_await_elidable]] task { + struct promise_type { + std::coroutine_handle<> continuation = nullptr; + + task get_return_object() noexcept { + return {std::coroutine_handle<promise_type>::from_promise(*this)}; + } + + std::suspend_never initial_suspend() noexcept { return {}; } + std::suspend_never final_suspend() noexcept { return {}; } + + void return_void() noexcept { + if (continuation) + continuation.resume(); + } + + void unhandled_exception() noexcept { __builtin_abort(); } + }; + + std::coroutine_handle<promise_type> handle; + + bool await_ready() noexcept { return false; } + + void await_suspend(std::coroutine_handle<> h) noexcept { + handle.promise().continuation = h; + } + + void await_resume() noexcept {} +}; + +task callee(gate &g, int &value) { + co_await g; + value = 42; +} + +task caller(gate &g, int &value, bool &finished) { + co_await callee(g, value); + finished = true; +} + +// CHECK-LABEL: define internal void @_Z6calleeR4gateRi.resume( +// CHECK: %[[DESTROY_ADDR:.+]] = getelementptr inbounds{{.*}} i8, ptr %{{.+}}, i64 8 +// CHECK-NEXT: %[[DESTROY:.+]] = load ptr, ptr %[[DESTROY_ADDR]] +// CHECK-NEXT: %[[IS_ELIDED:.+]] = icmp eq ptr %[[DESTROY]], @_Z6calleeR4gateRi.cleanup +// CHECK: store i32 42, +// CHECK: br i1 %[[IS_ELIDED]], label %[[CORO_END:.+]], label %[[CORO_FREE:.+]] +// CHECK: [[CORO_FREE]]: +// CHECK-NEXT: tail call void @_Zdl +// CHECK: [[CORO_END]]: +// CHECK-NEXT: ret void + +// CHECK-LABEL: define internal {{.*}}void @_Z6calleeR4gateRi.destroy( +// CHECK: call void @_Zdl + +// CHECK-LABEL: define internal {{.*}}void @_Z6calleeR4gateRi.cleanup( +// CHECK-NOT: call void @_Zdl +// CHECK: ret void diff --git a/llvm/include/llvm/Transforms/Coroutines/CoroShape.h b/llvm/include/llvm/Transforms/Coroutines/CoroShape.h index 28931e3260e68..534e63f28ddc1 100644 --- a/llvm/include/llvm/Transforms/Coroutines/CoroShape.h +++ b/llvm/include/llvm/Transforms/Coroutines/CoroShape.h @@ -112,6 +112,7 @@ struct Shape { unsigned IndexOffset; bool HasFinalSuspend; bool HasUnwindCoroEnd; + bool HasCoroElideNoAllocVariant; }; struct RetconLoweringStorage { diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp index 7915fbf4cce05..c788c1623587a 100644 --- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp @@ -164,6 +164,50 @@ static void maybeFreeRetconStorage(IRBuilder<> &Builder, Shape.emitDealloc(Builder, FramePtr, CG); } +/// Create a pointer to the switch destroy function field in the coroutine +/// frame. +static Value *createSwitchDestroyPtr(const coro::Shape &Shape, + IRBuilder<> &Builder, Value *FramePtr) { + auto *Offset = ConstantInt::get(Type::getInt64Ty(FramePtr->getContext()), + Shape.SwitchLowering.DestroyOffset); + return Builder.CreateInBoundsPtrAdd(FramePtr, Offset, "destroy.addr"); +} + +/// Make resume-clone coro.free conditional on whether the frame is elided. +/// +/// The destroy slot holds the cleanup clone for an elided frame and the destroy +/// clone for a heap frame. Load it before user code can reentrantly destroy the +/// enclosing caller frame, then use the cached comparison to suppress only the +/// deallocation. The resume clone has already performed the shared coroutine +/// cleanup, so calling either clone here would run that cleanup twice. +static void replaceSwitchResumeCoroFree(const coro::Shape &Shape, + Function &Resume, Function &Cleanup) { + Value *FramePtr = Resume.getArg(0); + IRBuilder<> EntryBuilder(Resume.getEntryBlock().getTerminator()); + Value *DestroyAddr = createSwitchDestroyPtr(Shape, EntryBuilder, FramePtr); + Value *DestroyFn = EntryBuilder.CreateLoad(Shape.getSwitchResumePointerType(), + DestroyAddr, "destroy"); + Value *CleanupFn = + EntryBuilder.CreatePointerCast(&Cleanup, DestroyFn->getType()); + Value *IsElided = + EntryBuilder.CreateICmpEQ(DestroyFn, CleanupFn, "is.elided"); + + SmallVector<CoroFreeInst *, 4> CoroFrees; + for (User *U : FramePtr->users()) { + if (auto *CF = dyn_cast<CoroFreeInst>(U)) + CoroFrees.push_back(CF); + } + + for (CoroFreeInst *CF : CoroFrees) { + IRBuilder<> Builder(CF); + auto *Null = ConstantPointerNull::get(cast<PointerType>(CF->getType())); + Value *Replacement = + Builder.CreateSelect(IsElided, Null, FramePtr, "coro.free"); + CF->replaceAllUsesWith(Replacement); + CF->eraseFromParent(); + } +} + /// Replace an llvm.coro.end.async. /// Will inline the must tail call function call if there is one. /// \returns true if cleanup of the coro.end block is needed, false otherwise. @@ -1382,6 +1426,9 @@ struct SwitchCoroutineSplitter { auto *CleanupClone = coro::SwitchCloner::createClone( F, ".cleanup", Shape, coro::CloneKind::SwitchCleanup, TTI); + if (Shape.SwitchLowering.HasCoroElideNoAllocVariant) + replaceSwitchResumeCoroFree(Shape, *ResumeClone, *CleanupClone); + postSplitCleanup(*ResumeClone); postSplitCleanup(*DestroyClone); postSplitCleanup(*CleanupClone); @@ -2014,6 +2061,9 @@ static void doSplitCoroutine(Function &F, SmallVectorImpl<Function *> &Clones, bool shouldCreateNoAllocVariant = !isNoSuspendCoroutine && Shape.ABI == coro::ABI::Switch && hasSafeElideCaller(F) && !F.hasFnAttribute(llvm::Attribute::NoInline); + if (Shape.ABI == coro::ABI::Switch) + Shape.SwitchLowering.HasCoroElideNoAllocVariant = + shouldCreateNoAllocVariant; // If there are no suspend points, no split required, just remove // the allocation and deallocation blocks, they are not needed. @@ -2052,10 +2102,22 @@ static LazyCallGraph::SCC &updateCallGraphAfterCoroutineSplit( if (!Clones.empty()) { switch (Shape.ABI) { case coro::ABI::Switch: - // Each clone in the Switch lowering is independent of the other clones. - // Let the LazyCallGraph know about each one separately. - for (Function *Clone : Clones) - CG.addSplitFunction(N.getFunction(), *Clone); + // The resume clone's elided-frame check holds a reference to the cleanup + // clone. Add the cleanup clone first, so populating the resume node does + // not materialize an unregistered cleanup node. + if (Shape.SwitchLowering.HasCoroElideNoAllocVariant) { + assert(Clones.size() >= 3 && "expected switch coroutine clones"); + CG.addSplitFunction(N.getFunction(), *Clones[2]); + CG.addSplitFunction(N.getFunction(), *Clones[1]); + CG.addSplitFunction(N.getFunction(), *Clones[0]); + for (Function *Clone : drop_begin(Clones, 3)) + CG.addSplitFunction(N.getFunction(), *Clone); + } else { + // Each clone in the Switch lowering is independent of the other + // clones. Let the LazyCallGraph know about each one separately. + for (Function *Clone : Clones) + CG.addSplitFunction(N.getFunction(), *Clone); + } break; case coro::ABI::Async: case coro::ABI::Retcon: diff --git a/llvm/lib/Transforms/Coroutines/Coroutines.cpp b/llvm/lib/Transforms/Coroutines/Coroutines.cpp index a922099a1f43f..2ecf66d18b5d3 100644 --- a/llvm/lib/Transforms/Coroutines/Coroutines.cpp +++ b/llvm/lib/Transforms/Coroutines/Coroutines.cpp @@ -294,6 +294,7 @@ void coro::Shape::analyze(Function &F, ABI = coro::ABI::Switch; SwitchLowering.HasFinalSuspend = HasFinalSuspend; SwitchLowering.HasUnwindCoroEnd = HasUnwindCoroEnd; + SwitchLowering.HasCoroElideNoAllocVariant = false; auto SwitchId = getSwitchCoroId(); SwitchLowering.ResumeSwitch = nullptr; diff --git a/llvm/test/Transforms/Coroutines/coro-split-00.ll b/llvm/test/Transforms/Coroutines/coro-split-00.ll index 727b9b2b9776e..8a8f65de97f0c 100644 --- a/llvm/test/Transforms/Coroutines/coro-split-00.ll +++ b/llvm/test/Transforms/Coroutines/coro-split-00.ll @@ -51,11 +51,15 @@ entry: ; CHECK: ret ptr %hdl ; CHECK-LABEL: @f.resume({{.*}}) { +; CHECK: %[[DESTROY_ADDR:.+]] = getelementptr inbounds i8, ptr %hdl, i64 8 +; CHECK-NEXT: %[[DESTROY:.+]] = load ptr, ptr %[[DESTROY_ADDR]] +; CHECK-NEXT: %[[IS_ELIDED:.+]] = icmp eq ptr %[[DESTROY]], @f.cleanup ; CHECK-NOT: call ptr @malloc ; CHECK-NOT: call void @print(i32 0) ; CHECK: call void @print(i32 1) ; CHECK-NOT: call void @print(i32 0) -; CHECK: call void @free( +; CHECK: %[[CORO_FREE:.+]] = select i1 %[[IS_ELIDED]], ptr null, ptr %hdl +; CHECK-NEXT: call void @free(ptr %[[CORO_FREE]]) ; CHECK: ret void ; CHECK-LABEL: @f.destroy({{.*}}) { diff --git a/llvm/test/Transforms/Coroutines/coro-split-addrspace.ll b/llvm/test/Transforms/Coroutines/coro-split-addrspace.ll index 24db35d141b35..fe08a7dd8b525 100644 --- a/llvm/test/Transforms/Coroutines/coro-split-addrspace.ll +++ b/llvm/test/Transforms/Coroutines/coro-split-addrspace.ll @@ -52,11 +52,15 @@ entry: ; CHECK: ret ptr %hdl ; CHECK-LABEL: @f.resume({{.*}}) addrspace(200) { +; CHECK: %[[DESTROY_ADDR:.+]] = getelementptr inbounds i8, ptr %hdl, i64 8 +; CHECK-NEXT: %[[DESTROY:.+]] = load ptr, ptr %[[DESTROY_ADDR]] +; CHECK-NEXT: %[[IS_ELIDED:.+]] = icmp eq ptr %[[DESTROY]], addrspacecast (ptr addrspace(200) @f.cleanup to ptr) ; CHECK-NOT: call ptr @malloc ; CHECK-NOT: call void @print(i32 0) ; CHECK: call void @print(i32 1) ; CHECK-NOT: call void @print(i32 0) -; CHECK: call void @free( +; CHECK: %[[CORO_FREE:.+]] = select i1 %[[IS_ELIDED]], ptr null, ptr %hdl +; CHECK-NEXT: call void @free(ptr %[[CORO_FREE]]) ; CHECK: ret void ; CHECK-LABEL: @f.destroy({{.*}}) addrspace(200) { diff --git a/llvm/test/Transforms/Coroutines/coro-split-resume-fallthrough-destroy-slot.ll b/llvm/test/Transforms/Coroutines/coro-split-resume-fallthrough-destroy-slot.ll new file mode 100644 index 0000000000000..9641a7253a5a0 --- /dev/null +++ b/llvm/test/Transforms/Coroutines/coro-split-resume-fallthrough-destroy-slot.ll @@ -0,0 +1,84 @@ +; Tests that a switch coroutine resume clone which falls through to coro.end +; suppresses only the deallocation for a stack-elided frame. This matters for +; allocation elision: the frame slot contains the cleanup clone in that case. +; +; RUN: opt < %s -passes='cgscc(coro-split),simplifycfg,early-cse' -S | FileCheck %s + +define ptr @f() presplitcoroutine { +entry: + %id = call token @llvm.coro.id(i32 0, ptr null, ptr @f, ptr null) + %need.alloc = call i1 @llvm.coro.alloc(token %id) + br i1 %need.alloc, label %dyn.alloc, label %begin + +dyn.alloc: + %size = call i32 @llvm.coro.size.i32() + %alloc = call ptr @malloc(i32 %size) + br label %begin + +begin: + %phi = phi ptr [ null, %entry ], [ %alloc, %dyn.alloc ] + %hdl = call ptr @llvm.coro.begin(token %id, ptr %phi) + call void @print(i32 0) + %0 = call i8 @llvm.coro.suspend(token none, i1 false) + switch i8 %0, label %suspend [i8 0, label %resume + i8 1, label %cleanup] + +resume: + call void @print(i32 1) + br label %cleanup + +cleanup: + %mem = call ptr @llvm.coro.free(token %id, ptr %hdl) + call void @free(ptr %mem) + br label %suspend + +suspend: + call void @llvm.coro.end(ptr %hdl, i1 false, token none) + ret ptr %hdl +} + +define void @caller() presplitcoroutine { +entry: + %ptr = call ptr @f() #0 + ret void +} + +; CHECK-LABEL: define ptr @f( +; CHECK: %[[NEED_ALLOC:.+]] = call i1 @llvm.coro.alloc( +; CHECK: %[[DESTROY_OR_CLEANUP:.+]] = select i1 %[[NEED_ALLOC]], ptr @f.destroy, ptr @f.cleanup +; CHECK: %[[DESTROY_ADDR:.+]] = getelementptr inbounds i8, ptr %hdl, i64 8 +; CHECK-NEXT: store ptr %[[DESTROY_OR_CLEANUP]], ptr %[[DESTROY_ADDR]] + +; CHECK-LABEL: define internal void @f.resume( +; CHECK: %[[DESTROY_ADDR:.+]] = getelementptr inbounds i8, ptr %hdl, i64 8 +; CHECK-NEXT: %[[DESTROY:.+]] = load ptr, ptr %[[DESTROY_ADDR]] +; CHECK-NEXT: %[[IS_ELIDED:.+]] = icmp eq ptr %[[DESTROY]], @f.cleanup +; CHECK: call void @print(i32 1) +; CHECK: %[[CORO_FREE:.+]] = select i1 %[[IS_ELIDED]], ptr null, ptr %hdl +; CHECK-NEXT: call void @free(ptr %[[CORO_FREE]]) +; CHECK-NEXT: ret void + +; CHECK-LABEL: define internal void @f.destroy( +; CHECK: call void @free( + +; CHECK-LABEL: define internal void @f.cleanup( +; CHECK-NOT: call void @free( +; CHECK: ret void + +; CHECK-LABEL: define internal ptr @f.noalloc( +; CHECK: %[[NOALLOC_DESTROY_ADDR:.+]] = getelementptr inbounds i8, ptr %{{.+}}, i64 8 +; CHECK-NEXT: store ptr @f.cleanup, ptr %[[NOALLOC_DESTROY_ADDR]] + +declare ptr @llvm.coro.free(token, ptr) +declare i32 @llvm.coro.size.i32() +declare i8 @llvm.coro.suspend(token, i1) +declare token @llvm.coro.id(i32, ptr, ptr, ptr) +declare i1 @llvm.coro.alloc(token) +declare ptr @llvm.coro.begin(token, ptr) +declare void @llvm.coro.end(ptr, i1, token) + +declare noalias ptr @malloc(i32) allockind("alloc,uninitialized") "alloc-family"="malloc" +declare void @print(i32) +declare void @free(ptr) willreturn allockind("free") "alloc-family"="malloc" + +attributes #0 = { coro_elide_safe } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
