Author: Chuanqi Xu Date: 2023-08-28T13:21:17+08:00 New Revision: 572cc8d38f938eb2769907c17137a10a408d9bfc
URL: https://github.com/llvm/llvm-project/commit/572cc8d38f938eb2769907c17137a10a408d9bfc DIFF: https://github.com/llvm/llvm-project/commit/572cc8d38f938eb2769907c17137a10a408d9bfc.diff LOG: Revert "[C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty" This reverts commit 9d9c25f81456aace2bec4b58498a420e650007d9. This reverts commit 19ab2664ad3182ffa8fe3a95bb19765e4ae84653. This reverts commit c4672454743e942f148a1aff1e809dae73e464f6. As the issue https://github.com/llvm/llvm-project/issues/65018 shows, the previous fix introduce a regression actually. So this commit reverts the fix by our policies. Added: Modified: clang/docs/ReleaseNotes.rst clang/lib/CodeGen/CGCall.cpp clang/lib/CodeGen/CGCoroutine.cpp clang/lib/CodeGen/CodeGenFunction.h Removed: clang/test/CodeGenCoroutines/coro-awaiter-noinline-suspend.cpp clang/test/CodeGenCoroutines/pr56301.cpp ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 6dc3c1c5fbcef8..767861df912998 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -174,13 +174,6 @@ Bug Fixes in This Version ``abs`` builtins. (`#45129 <https://github.com/llvm/llvm-project/issues/45129>`_, `#45794 <https://github.com/llvm/llvm-project/issues/45794>`_) -- Fixed an issue where accesses to the local variables of a coroutine during - ``await_suspend`` could be misoptimized, including accesses to the awaiter - object itself. - (`#56301 <https://github.com/llvm/llvm-project/issues/56301>`_) - The current solution may bring performance regressions if the awaiters have - non-static data members. See - `#64945 <https://github.com/llvm/llvm-project/issues/64945>`_ for details. - Clang now prints unnamed members in diagnostic messages instead of giving an empty ''. Fixes (`#63759 <https://github.com/llvm/llvm-project/issues/63759>`_) diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp index 8acb03cd5cfc4d..e323ddf8bedae9 100644 --- a/clang/lib/CodeGen/CGCall.cpp +++ b/clang/lib/CodeGen/CGCall.cpp @@ -5527,34 +5527,6 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::AlwaysInline); } - // The await_suspend call performed by co_await is essentially asynchronous - // to the execution of the coroutine. Inlining it normally into an unsplit - // coroutine can cause miscompilation because the coroutine CFG misrepresents - // the true control flow of the program: things that happen in the - // await_suspend are not guaranteed to happen prior to the resumption of the - // coroutine, and things that happen after the resumption of the coroutine - // (including its exit and the potential deallocation of the coroutine frame) - // are not guaranteed to happen only after the end of await_suspend. - // - // The short-term solution to this problem is to mark the call as uninlinable. - // But we don't want to do this if the call is known to be trivial, which is - // very common. - // - // The long-term solution may introduce patterns like: - // - // call @llvm.coro.await_suspend(ptr %awaiter, ptr %handle, - // ptr @awaitSuspendFn) - // - // Then it is much easier to perform the safety analysis in the middle end. - // If it is safe to inline the call to awaitSuspend, we can replace it in the - // CoroEarly pass. Otherwise we could replace it in the CoroSplit pass. - // - // If the `await_suspend()` function is marked as `always_inline` explicitly, - // we should give the user the right to control the codegen. - if (inSuspendBlock() && mayCoroHandleEscape() && - !TargetDecl->hasAttr<AlwaysInlineAttr>()) - Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoInline); - // Disable inlining inside SEH __try blocks. if (isSEHTryScope()) { Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoInline); diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp index 2614596312f63d..cf260570510176 100644 --- a/clang/lib/CodeGen/CGCoroutine.cpp +++ b/clang/lib/CodeGen/CGCoroutine.cpp @@ -139,36 +139,6 @@ static bool memberCallExpressionCanThrow(const Expr *E) { return true; } -/// Return true when the coroutine handle may escape from the await-suspend -/// (`awaiter.await_suspend(std::coroutine_handle)` expression). -/// Return false only when the coroutine wouldn't escape in the await-suspend -/// for sure. -/// -/// While it is always safe to return true, return falses can bring better -/// performances. -/// -/// See https://github.com/llvm/llvm-project/issues/56301 and -/// https://reviews.llvm.org/D157070 for the example and the full discussion. -/// -/// FIXME: It will be much better to perform such analysis in the middle end. -/// See the comments in `CodeGenFunction::EmitCall` for example. -static bool MayCoroHandleEscape(CoroutineSuspendExpr const &S) { - CXXRecordDecl *Awaiter = - S.getCommonExpr()->getType().getNonReferenceType()->getAsCXXRecordDecl(); - - // Return true conservatively if the awaiter type is not a record type. - if (!Awaiter) - return true; - - // In case the awaiter type is empty, the suspend wouldn't leak the coroutine - // handle. - // - // TODO: We can improve this by looking into the implementation of - // await-suspend and see if the coroutine handle is passed to foreign - // functions. - return !Awaiter->field_empty(); -} - // Emit suspend expression which roughly looks like: // // auto && x = CommonExpr(); @@ -229,11 +199,8 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr}); CGF.CurCoro.InSuspendBlock = true; - CGF.CurCoro.MayCoroHandleEscape = MayCoroHandleEscape(S); auto *SuspendRet = CGF.EmitScalarExpr(S.getSuspendExpr()); CGF.CurCoro.InSuspendBlock = false; - CGF.CurCoro.MayCoroHandleEscape = false; - if (SuspendRet != nullptr && SuspendRet->getType()->isIntegerTy(1)) { // Veto suspension if requested by bool returning await_suspend. BasicBlock *RealSuspendBlock = diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h index 215166f566ea65..60f2f21de53ab9 100644 --- a/clang/lib/CodeGen/CodeGenFunction.h +++ b/clang/lib/CodeGen/CodeGenFunction.h @@ -334,7 +334,6 @@ class CodeGenFunction : public CodeGenTypeCache { struct CGCoroInfo { std::unique_ptr<CGCoroData> Data; bool InSuspendBlock = false; - bool MayCoroHandleEscape = false; CGCoroInfo(); ~CGCoroInfo(); }; @@ -348,10 +347,6 @@ class CodeGenFunction : public CodeGenTypeCache { return isCoroutine() && CurCoro.InSuspendBlock; } - bool mayCoroHandleEscape() const { - return isCoroutine() && CurCoro.MayCoroHandleEscape; - } - /// CurGD - The GlobalDecl for the current function being compiled. GlobalDecl CurGD; diff --git a/clang/test/CodeGenCoroutines/coro-awaiter-noinline-suspend.cpp b/clang/test/CodeGenCoroutines/coro-awaiter-noinline-suspend.cpp deleted file mode 100644 index fe66d67e6e3557..00000000000000 --- a/clang/test/CodeGenCoroutines/coro-awaiter-noinline-suspend.cpp +++ /dev/null @@ -1,224 +0,0 @@ -// Tests that we can mark await-suspend as noinline correctly. -// -// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s \ -// RUN: -disable-llvm-passes | FileCheck %s - -#include "Inputs/coroutine.h" - -struct Task { - struct promise_type { - struct FinalAwaiter { - bool await_ready() const noexcept { return false; } - template <typename PromiseType> - std::coroutine_handle<> await_suspend(std::coroutine_handle<PromiseType> h) noexcept { - return h.promise().continuation; - } - void await_resume() noexcept {} - }; - - Task get_return_object() noexcept { - return std::coroutine_handle<promise_type>::from_promise(*this); - } - - std::suspend_always initial_suspend() noexcept { return {}; } - FinalAwaiter final_suspend() noexcept { return {}; } - void unhandled_exception() noexcept {} - void return_void() noexcept {} - - std::coroutine_handle<> continuation; - }; - - Task(std::coroutine_handle<promise_type> handle); - ~Task(); - -private: - std::coroutine_handle<promise_type> handle; -}; - -struct StatefulAwaiter { - int value; - bool await_ready() const noexcept { return false; } - template <typename PromiseType> - void await_suspend(std::coroutine_handle<PromiseType> h) noexcept {} - void await_resume() noexcept {} -}; - -typedef std::suspend_always NoStateAwaiter; -using AnotherStatefulAwaiter = StatefulAwaiter; - -template <class T> -struct TemplatedAwaiter { - T value; - bool await_ready() const noexcept { return false; } - template <typename PromiseType> - void await_suspend(std::coroutine_handle<PromiseType> h) noexcept {} - void await_resume() noexcept {} -}; - - -class Awaitable {}; -StatefulAwaiter operator co_await(Awaitable) { - return StatefulAwaiter{}; -} - -StatefulAwaiter GlobalAwaiter; -class Awaitable2 {}; -StatefulAwaiter& operator co_await(Awaitable2) { - return GlobalAwaiter; -} - -struct AlwaysInlineStatefulAwaiter { - void* value; - bool await_ready() const noexcept { return false; } - - template <typename PromiseType> - __attribute__((always_inline)) - void await_suspend(std::coroutine_handle<PromiseType> h) noexcept {} - - void await_resume() noexcept {} -}; - -Task testing() { - co_await std::suspend_always{}; - co_await StatefulAwaiter{}; - co_await AnotherStatefulAwaiter{}; - - // Test lvalue case. - StatefulAwaiter awaiter; - co_await awaiter; - - // The explicit call to await_suspend is not considered suspended. - awaiter.await_suspend(std::coroutine_handle<void>::from_address(nullptr)); - - co_await TemplatedAwaiter<int>{}; - TemplatedAwaiter<int> TemplatedAwaiterInstace; - co_await TemplatedAwaiterInstace; - - co_await Awaitable{}; - co_await Awaitable2{}; - - co_await AlwaysInlineStatefulAwaiter{}; -} - -// CHECK-LABEL: @_Z7testingv - -// Check `co_await __promise__.initial_suspend();` Since it returns std::suspend_always, -// which is an empty class, we shouldn't generate optimization blocker for it. -// CHECK: call token @llvm.coro.save -// CHECK: call void @_ZNSt14suspend_always13await_suspendESt16coroutine_handleIvE{{.*}}#[[NORMAL_ATTR:[0-9]+]] - -// Check the `co_await std::suspend_always{};` expression. We shouldn't emit the optimization -// blocker for it since it is an empty class. -// CHECK: call token @llvm.coro.save -// CHECK: call void @_ZNSt14suspend_always13await_suspendESt16coroutine_handleIvE{{.*}}#[[NORMAL_ATTR]] - -// Check `co_await StatefulAwaiter{};`. We need to emit the optimization blocker since -// the awaiter is not empty. -// CHECK: call token @llvm.coro.save -// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR:[0-9]+]] - -// Check `co_await AnotherStatefulAwaiter{};` to make sure that we can handle TypedefTypes. -// CHECK: call token @llvm.coro.save -// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]] - -// Check `co_await awaiter;` to make sure we can handle lvalue cases. -// CHECK: call token @llvm.coro.save -// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]] - -// Check `awaiter.await_suspend(...)` to make sure the explicit call the await_suspend won't be marked as noinline -// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIvEEvSt16coroutine_handleIT_E{{.*}}#[[NORMAL_ATTR]] - -// Check `co_await TemplatedAwaiter<int>{};` to make sure we can handle specialized template -// type. -// CHECK: call token @llvm.coro.save -// CHECK: call void @_ZN16TemplatedAwaiterIiE13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]] - -// Check `co_await TemplatedAwaiterInstace;` to make sure we can handle the lvalue from -// specialized template type. -// CHECK: call token @llvm.coro.save -// CHECK: call void @_ZN16TemplatedAwaiterIiE13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]] - -// Check `co_await Awaitable{};` to make sure we can handle awaiter returned by -// `operator co_await`; -// CHECK: call token @llvm.coro.save -// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]] - -// Check `co_await Awaitable2{};` to make sure we can handle awaiter returned by -// `operator co_await` which returns a reference; -// CHECK: call token @llvm.coro.save -// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]] - -// Check `co_await AlwaysInlineStatefulAwaiter{};` to make sure user can force the await_suspend function to get inlined. -// CHECK: call token @llvm.coro.save -// CHECK: call void @_ZN27AlwaysInlineStatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NORMAL_ATTR]] - -// Check `co_await __promise__.final_suspend();`. We don't emit an blocker here since it is -// empty. -// CHECK: call token @llvm.coro.save -// CHECK: call ptr @_ZN4Task12promise_type12FinalAwaiter13await_suspendIS0_EESt16coroutine_handleIvES3_IT_E{{.*}}#[[NORMAL_ATTR]] - -struct AwaitTransformTask { - struct promise_type { - struct FinalAwaiter { - bool await_ready() const noexcept { return false; } - template <typename PromiseType> - std::coroutine_handle<> await_suspend(std::coroutine_handle<PromiseType> h) noexcept { - return h.promise().continuation; - } - void await_resume() noexcept {} - }; - - AwaitTransformTask get_return_object() noexcept { - return std::coroutine_handle<promise_type>::from_promise(*this); - } - - std::suspend_always initial_suspend() noexcept { return {}; } - FinalAwaiter final_suspend() noexcept { return {}; } - void unhandled_exception() noexcept {} - void return_void() noexcept {} - - template <typename Awaitable> - auto await_transform(Awaitable &&awaitable) { - return awaitable; - } - - std::coroutine_handle<> continuation; - }; - - AwaitTransformTask(std::coroutine_handle<promise_type> handle); - ~AwaitTransformTask(); - -private: - std::coroutine_handle<promise_type> handle; -}; - -struct awaitableWithGetAwaiter { - bool await_ready() const noexcept { return false; } - template <typename PromiseType> - void await_suspend(std::coroutine_handle<PromiseType> h) noexcept {} - void await_resume() noexcept {} -}; - -AwaitTransformTask testingWithAwaitTransform() { - co_await awaitableWithGetAwaiter{}; -} - -// CHECK-LABEL: @_Z25testingWithAwaitTransformv - -// Init suspend -// CHECK: call token @llvm.coro.save -// CHECK-NOT: call void @llvm.coro.opt.blocker( -// CHECK: call void @_ZNSt14suspend_always13await_suspendESt16coroutine_handleIvE{{.*}}#[[NORMAL_ATTR]] - -// Check `co_await awaitableWithGetAwaiter{};`. -// CHECK: call token @llvm.coro.save -// CHECK-NOT: call void @llvm.coro.opt.blocker( -// Check call void @_ZN23awaitableWithGetAwaiter13await_suspendIN18AwaitTransformTask12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NORMAL_ATTR]] - -// Final suspend -// CHECK: call token @llvm.coro.save -// CHECK-NOT: call void @llvm.coro.opt.blocker( -// CHECK: call ptr @_ZN18AwaitTransformTask12promise_type12FinalAwaiter13await_suspendIS0_EESt16coroutine_handleIvES3_IT_E{{.*}}#[[NORMAL_ATTR]] - -// CHECK-NOT: attributes #[[NORMAL_ATTR]] = noinline -// CHECK: attributes #[[NOINLINE_ATTR]] = {{.*}}noinline diff --git a/clang/test/CodeGenCoroutines/pr56301.cpp b/clang/test/CodeGenCoroutines/pr56301.cpp deleted file mode 100644 index cd851c0b815db9..00000000000000 --- a/clang/test/CodeGenCoroutines/pr56301.cpp +++ /dev/null @@ -1,85 +0,0 @@ -// An end-to-end test to make sure things get processed correctly. -// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s -O3 | \ -// RUN: FileCheck %s - -#include "Inputs/coroutine.h" - -struct SomeAwaitable { - // Resume the supplied handle once the awaitable becomes ready, - // returning a handle that should be resumed now for the sake of symmetric transfer. - // If the awaitable is already ready, return an empty handle without doing anything. - // - // Defined in another translation unit. Note that this may contain - // code that synchronizees with another thread. - std::coroutine_handle<> Register(std::coroutine_handle<>); -}; - -// Defined in another translation unit. -void DidntSuspend(); - -struct Awaiter { - SomeAwaitable&& awaitable; - bool suspended; - - bool await_ready() { return false; } - - std::coroutine_handle<> await_suspend(const std::coroutine_handle<> h) { - // Assume we will suspend unless proven otherwise below. We must do - // this *before* calling Register, since we may be destroyed by another - // thread asynchronously as soon as we have registered. - suspended = true; - - // Attempt to hand off responsibility for resuming/destroying the coroutine. - const auto to_resume = awaitable.Register(h); - - if (!to_resume) { - // The awaitable is already ready. In this case we know that Register didn't - // hand off responsibility for the coroutine. So record the fact that we didn't - // actually suspend, and tell the compiler to resume us inline. - suspended = false; - return h; - } - - // Resume whatever Register wants us to resume. - return to_resume; - } - - void await_resume() { - // If we didn't suspend, make note of that fact. - if (!suspended) { - DidntSuspend(); - } - } -}; - -struct MyTask{ - struct promise_type { - MyTask get_return_object() { return {}; } - std::suspend_never initial_suspend() { return {}; } - std::suspend_always final_suspend() noexcept { return {}; } - void unhandled_exception(); - - Awaiter await_transform(SomeAwaitable&& awaitable) { - return Awaiter{static_cast<SomeAwaitable&&>(awaitable)}; - } - }; -}; - -MyTask FooBar() { - co_await SomeAwaitable(); -} - -// CHECK-LABEL: @_Z6FooBarv -// CHECK: %[[to_resume:.*]] = {{.*}}call ptr @_ZN13SomeAwaitable8RegisterESt16coroutine_handleIvE -// CHECK-NEXT: %[[to_bool:.*]] = icmp eq ptr %[[to_resume]], null -// CHECK-NEXT: br i1 %[[to_bool]], label %[[then:.*]], label %[[else:.*]] - -// CHECK: [[then]]: -// We only access the coroutine frame conditionally as the sources did. -// CHECK: store i8 0, -// CHECK-NEXT: br label %[[else]] - -// CHECK: [[else]]: -// No more access to the coroutine frame until suspended. -// CHECK-NOT: store -// CHECK: } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits