diff --git a/clang/test/CodeGenCoroutines/coro-halo.cpp b/clang/test/CodeGenCoroutines/coro-halo.cpp index 6244f130b7be2..e75bedaf81fa2 100644 --- a/clang/test/CodeGenCoroutines/coro-halo.cpp +++ b/clang/test/CodeGenCoroutines/coro-halo.cpp @@ -1,5 +1,7 @@ // This tests that the coroutine heap allocation elision optimization could happen succesfully. // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -O2 -emit-llvm %s -o - | FileCheck %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -O2 -emit-llvm %s \ +// RUN: -fcxx-exceptions -fexceptions -o - | FileCheck %s #include "Inputs/coroutine.h" #include "Inputs/numeric.h" diff --git a/clang/test/CodeGenCoroutines/pr59723.cpp b/clang/test/CodeGenCoroutines/pr59723.cpp new file mode 100644 index 0000000000000..7fc9995f417ac --- /dev/null +++ b/clang/test/CodeGenCoroutines/pr59723.cpp @@ -0,0 +1,237 @@ +// This is reduced test case from https://github.com/llvm/llvm-project/issues/59723. +// This is not a minimal reproducer intentionally to check the compiler's ability. +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -fcxx-exceptions\ +// RUN: -fexceptions -O2 -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/coroutine.h" + +// executor and operation base + +class bug_any_executor; + +struct bug_async_op_base +{ + void invoke(); + +protected: + + ~bug_async_op_base() = default; +}; + +class bug_any_executor +{ + using op_type = bug_async_op_base; + +public: + + virtual ~bug_any_executor() = default; + + // removing noexcept enables clang to find that the pointer has escaped + virtual void post(op_type& op) noexcept = 0; + + virtual void wait() noexcept = 0; +}; + +class bug_thread_executor : public bug_any_executor +{ + +public: + + void start() + { + + } + + ~bug_thread_executor() + { + } + + // although this implementation is not realy noexcept due to allocation but I have a real one that is and required to be noexcept + virtual void post(bug_async_op_base& op) noexcept override; + + virtual void wait() noexcept override + { + + } +}; + +// task and promise + +struct bug_final_suspend_notification +{ + virtual std::coroutine_handle<> get_waiter() = 0; +}; + +class bug_task; + +class bug_task_promise +{ + friend bug_task; +public: + + bug_task get_return_object() noexcept; + + constexpr std::suspend_always initial_suspend() noexcept { return {}; } + + std::suspend_always final_suspend() noexcept + { + return {}; + } + + void unhandled_exception() noexcept; + + constexpr void return_void() const noexcept {} + + void get_result() const + { + + } +}; + +template +T exchange(T &&t, U &&u) { + T ret = t; + t = u; + return ret; +} + +class bug_task +{ + friend bug_task_promise; + using handle = std::coroutine_handle<>; + using promise_t = bug_task_promise; + + bug_task(handle coro, promise_t* p) noexcept : this_coro{ coro }, this_promise{ p } + { + + } + +public: + using promise_type = bug_task_promise; + + bug_task(bug_task&& other) noexcept + : this_coro{ exchange(other.this_coro, nullptr) }, this_promise{ exchange(other.this_promise, nullptr) } { + + } + + ~bug_task() + { + if (this_coro) + this_coro.destroy(); + } + + constexpr bool await_ready() const noexcept + { + return false; + } + + handle await_suspend(handle waiter) noexcept + { + return this_coro; + } + + void await_resume() + { + return this_promise->get_result(); + } + + handle this_coro; + promise_t* this_promise; +}; + +bug_task bug_task_promise::get_return_object() noexcept +{ + return { std::coroutine_handle::from_promise(*this), this }; +} + +// spawn operation and spawner + +template +class bug_spawn_op final : public bug_async_op_base, bug_final_suspend_notification +{ + Handler handler; + bug_task task_; + +public: + + bug_spawn_op(Handler handler, bug_task&& t) + : handler { handler }, task_{ static_cast(t) } {} + + virtual std::coroutine_handle<> get_waiter() override + { + handler(); + return std::noop_coroutine(); + } +}; + +class bug_spawner; + +struct bug_spawner_awaiter +{ + bug_spawner& s; + std::coroutine_handle<> waiter; + + bug_spawner_awaiter(bug_spawner& s) : s{ s } {} + + bool await_ready() const noexcept; + + void await_suspend(std::coroutine_handle<> coro); + + void await_resume() {} +}; + +class bug_spawner +{ + friend bug_spawner_awaiter; + + struct final_handler_t + { + bug_spawner& s; + + void operator()() + { + s.awaiter_->waiter.resume(); + } + }; + +public: + + bug_spawner(bug_any_executor& ex) : ex_{ ex } {} + + void spawn(bug_task&& t) { + using op_t = bug_spawn_op; + // move task into ptr + op_t* ptr = new op_t(final_handler_t{ *this }, static_cast(t)); + ++count_; + ex_.post(*ptr); // ptr escapes here thus task escapes but clang can't deduce that unless post() is not noexcept + } + + bug_spawner_awaiter wait() noexcept { return { *this }; } + +private: + bug_any_executor& ex_; // if bug_thread_executor& is used instead enables clang to detect the escape of the promise + bug_spawner_awaiter* awaiter_ = nullptr; + unsigned count_ = 0; +}; + +// test case + +bug_task bug_spawned_task(int id, int inc) +{ + co_return; +} + +struct A { + A(); +}; + +void throwing_fn(bug_spawner& s) { + s.spawn(bug_spawned_task(1, 2)); + throw A{}; +} + +// Check that the coroutine frame of bug_spawned_task are allocated from operator new. +// CHECK: define{{.*}}@_Z11throwing_fnR11bug_spawner +// CHECK-NOT: alloc +// CHECK: %[[CALL:.+]] = {{.*}}@_Znwm(i64{{.*}} 24) +// CHECK: store ptr @_Z16bug_spawned_taskii.resume, ptr %[[CALL]] diff --git a/llvm/lib/Transforms/Coroutines/CoroElide.cpp b/llvm/lib/Transforms/Coroutines/CoroElide.cpp index d78ab1c1ea284..d0606c15f3d5b 100644 --- a/llvm/lib/Transforms/Coroutines/CoroElide.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroElide.cpp @@ -194,12 +194,49 @@ bool Lowerer::hasEscapePath(const CoroBeginInst *CB, for (auto *DA : It->second) Visited.insert(DA->getParent()); + SmallPtrSet EscapingBBs; + for (auto *U : CB->users()) { + // The use from coroutine intrinsics are not a problem. + if (isa(U)) + continue; + + // Think all other usages may be an escaping candidate conservatively. + // + // Note that the major user of switch ABI coroutine (the C++) will store + // resume.fn, destroy.fn and the index to the coroutine frame immediately. + // So the parent of the coro.begin in C++ will be always escaping. + // Then we can't get any performance benefits for C++ by improving the + // precision of the method. + // + // The reason why we still judge it is we want to make LLVM Coroutine in + // switch ABIs to be self contained as much as possible instead of a + // by-product of C++20 Coroutines. + EscapingBBs.insert(cast(U)->getParent()); + } + + bool PotentiallyEscaped = false; + do { const auto *BB = Worklist.pop_back_val(); if (!Visited.insert(BB).second) continue; - if (TIs.count(BB)) - return true; + + // A Path insensitive marker to test whether the coro.begin escapes. + // It is intentional to make it path insensitive while it may not be + // precise since we don't want the process to be too slow. + PotentiallyEscaped |= EscapingBBs.count(BB); + + if (TIs.count(BB)) { + if (!BB->getTerminator()->isExceptionalTerminator() || PotentiallyEscaped) + return true; + + // If the function ends with the exceptional terminator, the memory used + // by the coroutine frame can be released by stack unwinding + // automatically. So we can think the coro.begin doesn't escape if it + // exits the function by exceptional terminator. + + continue; + } // Conservatively say that there is potentially a path. if (!--Limit) @@ -236,36 +273,36 @@ bool Lowerer::shouldElide(Function *F, DominatorTree &DT) const { // memory location storing that value and not the virtual register. SmallPtrSet Terminators; - // First gather all of the non-exceptional terminators for the function. + // First gather all of the terminators for the function. // Consider the final coro.suspend as the real terminator when the current // function is a coroutine. - for (BasicBlock &B : *F) { - auto *TI = B.getTerminator(); - if (TI->getNumSuccessors() == 0 && !TI->isExceptionalTerminator() && - !isa(TI)) - Terminators.insert(&B); - } + for (BasicBlock &B : *F) { + auto *TI = B.getTerminator(); + + if (TI->getNumSuccessors() != 0 || isa(TI)) + continue; + + Terminators.insert(&B); + } // Filter out the coro.destroy that lie along exceptional paths. SmallPtrSet ReferencedCoroBegins; for (const auto &It : DestroyAddr) { - // If there is any coro.destroy dominates all of the terminators for the - // coro.begin, we could know the corresponding coro.begin wouldn't escape. - for (Instruction *DA : It.second) { - if (llvm::all_of(Terminators, [&](auto *TI) { - return DT.dominates(DA, TI->getTerminator()); - })) { - ReferencedCoroBegins.insert(It.first); - break; - } - } - - // Whether there is any paths from coro.begin to Terminators which not pass - // through any of the coro.destroys. + // If every terminators is dominated by coro.destroy, we could know the + // corresponding coro.begin wouldn't escape. + // + // Otherwise hasEscapePath would decide whether there is any paths from + // coro.begin to Terminators which not pass through any of the + // coro.destroys. // // hasEscapePath is relatively slow, so we avoid to run it as much as // possible. - if (!ReferencedCoroBegins.count(It.first) && + if (llvm::all_of(Terminators, + [&](auto *TI) { + return llvm::any_of(It.second, [&](auto *DA) { + return DT.dominates(DA, TI->getTerminator()); + }); + }) || !hasEscapePath(It.first, Terminators)) ReferencedCoroBegins.insert(It.first); }