[llvm-branch-commits] [clang] 88bf774 - Revert "[C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty"

Chuanqi Xu via llvm-branch-commits llvm-branch-commits at lists.llvm.org
Mon Sep 18 00:14:57 PDT 2023


Author: Chuanqi Xu
Date: 2023-09-18T15:09:00+08:00
New Revision: 88bf774c565080e30e0a073676c316ab175303af

URL: https://github.com/llvm/llvm-project/commit/88bf774c565080e30e0a073676c316ab175303af
DIFF: https://github.com/llvm/llvm-project/commit/88bf774c565080e30e0a073676c316ab175303af.diff

LOG: Revert "[C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty"

This reverts commit f05226d7e38c36efe029a0eb4201b0843f81b5e8.

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 8ab7e065d218412..a4b61b9074d91e0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -711,10 +711,6 @@ Bug Fixes in This Version
 - Fix a hang on valid C code passing a function type as an argument to
   ``typeof`` to form a function declaration.
   (`#64713 <https://github.com/llvm/llvm-project/issues/64713>_`)
-- 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>`_)
 - Clang now correctly diagnoses ``function_needs_feature`` when always_inline
   callee has incompatible target features with caller.
 - Removed the linking of libraries when ``-r`` is passed to the driver on AIX.

diff  --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 0d1e9ad439b7dc9..6b8af9bf18c1fff 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -5487,30 +5487,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 (inSuspendBlock() && mayCoroHandleEscape())
-    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 810ae7d51ec10c2..8437cda79beb2a7 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 28ec2b970072109..8722fd4550e4a70 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 f935e256d9db99f..000000000000000
--- a/clang/test/CodeGenCoroutines/coro-awaiter-noinline-suspend.cpp
+++ /dev/null
@@ -1,207 +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;
-}
-
-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{};
-}
-
-// 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 __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 cd851c0b815db90..000000000000000
--- 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: }


        


More information about the llvm-branch-commits mailing list