Review fixes

fpasserby · fpasserby · commit 8ede407131c1 · 2024-03-04T23:00:31.000+01:00
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
@@ -5038,7 +5038,8 @@ class CoroutineSuspendExpr : public Expr {
   OpaqueValueExpr *OpaqueValue = nullptr;
 
 public:
-  enum SuspendReturnType { SuspendVoid, SuspendBool, SuspendHandle };
+  // These types correspond to the three C++ 'await_suspend' return variants
+  enum class SuspendReturnType { SuspendVoid, SuspendBool, SuspendHandle };
 
   CoroutineSuspendExpr(StmtClass SC, SourceLocation KeywordLoc, Expr *Operand,
                        Expr *Common, Expr *Ready, Expr *Suspend, Expr *Resume,
@@ -5109,10 +5110,12 @@ class CoroutineSuspendExpr : public Expr {
       return SuspendReturnType::SuspendVoid;
     if (SuspendType->isBooleanType())
       return SuspendReturnType::SuspendBool;
-    if (SuspendType->isVoidPointerType())
-      return SuspendReturnType::SuspendHandle;
 
-    llvm_unreachable("Unexpected await_suspend expression return type");
+    // Void pointer is the type of handle.address(), which is returned
+    // from the await suspend wrapper so that the temporary coroutine handle
+    // value won't go to the frame by mistake
+    assert(SuspendType->isVoidPointerType());
+    return SuspendReturnType::SuspendHandle;
   }
 
   SourceLocation getKeywordLoc() const { return KeywordLoc; }
diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp
@@ -182,8 +182,8 @@ static bool AwaitSuspendStmtCanThrow(const Stmt *S) {
 //   auto && x = CommonExpr();
 //   if (!x.await_ready()) {
 //      llvm_coro_save();
-//      llvm_coro_await_suspend(&x, frame, wrapper) (*)
-//      llvm_coro_suspend(); (**)
+//      llvm_coro_await_suspend(&x, frame, wrapper) (*) (**)
+//      llvm_coro_suspend(); (***)
 //   }
 //   x.await_resume();
 //
@@ -202,11 +202,11 @@ static bool AwaitSuspendStmtCanThrow(const Stmt *S) {
 //        return awaiter.await_suspend(handle);
 //      }
 //
-//      If x.await_suspend return type is bool, it allows to veto a suspend:
+//  (**) If x.await_suspend return type is bool, it allows to veto a suspend:
 //      if (x.await_suspend(...))
 //        llvm_coro_suspend();
 //
-//  (**) llvm_coro_suspend() encodes three possible continuations as
+//  (***) llvm_coro_suspend() encodes three possible continuations as
 //       a switch instruction:
 //
 //  %where-to = call i8 @llvm.coro.suspend(...)
@@ -250,14 +250,9 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
   auto *NullPtr = llvm::ConstantPointerNull::get(CGF.CGM.Int8PtrTy);
   auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr});
 
-  const auto AwaitSuspendCanThrow =
-      AwaitSuspendStmtCanThrow(S.getSuspendExpr());
-
   auto SuspendWrapper = CodeGenFunction(CGF.CGM).generateAwaitSuspendWrapper(
       CGF.CurFn->getName(), Prefix, S);
 
-  llvm::CallBase *SuspendRet = nullptr;
-
   CGF.CurCoro.InSuspendBlock = true;
 
   assert(CGF.CurCoro.Data && CGF.CurCoro.Data->CoroBegin &&
@@ -271,21 +266,26 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
   SuspendIntrinsicCallArgs.push_back(SuspendWrapper);
 
   const auto SuspendReturnType = S.getSuspendReturnType();
-  llvm::Intrinsic::ID IID;
+  llvm::Intrinsic::ID AwaitSuspendIID;
 
   switch (SuspendReturnType) {
-  case CoroutineSuspendExpr::SuspendVoid:
-    IID = llvm::Intrinsic::coro_await_suspend_void;
+  case CoroutineSuspendExpr::SuspendReturnType::SuspendVoid:
+    AwaitSuspendIID = llvm::Intrinsic::coro_await_suspend_void;
     break;
-  case CoroutineSuspendExpr::SuspendBool:
-    IID = llvm::Intrinsic::coro_await_suspend_bool;
+  case CoroutineSuspendExpr::SuspendReturnType::SuspendBool:
+    AwaitSuspendIID = llvm::Intrinsic::coro_await_suspend_bool;
     break;
-  case CoroutineSuspendExpr::SuspendHandle:
-    IID = llvm::Intrinsic::coro_await_suspend_handle;
+  case CoroutineSuspendExpr::SuspendReturnType::SuspendHandle:
+    AwaitSuspendIID = llvm::Intrinsic::coro_await_suspend_handle;
     break;
   }
 
-  llvm::Function *AwaitSuspendIntrinsic = CGF.CGM.getIntrinsic(IID);
+  llvm::Function *AwaitSuspendIntrinsic = CGF.CGM.getIntrinsic(AwaitSuspendIID);
+
+  const auto AwaitSuspendCanThrow =
+      AwaitSuspendStmtCanThrow(S.getSuspendExpr());
+
+  llvm::CallBase *SuspendRet = nullptr;
   // FIXME: add call attributes?
   if (AwaitSuspendCanThrow)
     SuspendRet =
@@ -298,10 +298,10 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
   CGF.CurCoro.InSuspendBlock = false;
 
   switch (SuspendReturnType) {
-  case CoroutineSuspendExpr::SuspendVoid:
+  case CoroutineSuspendExpr::SuspendReturnType::SuspendVoid:
     assert(SuspendRet->getType()->isVoidTy());
     break;
-  case CoroutineSuspendExpr::SuspendBool: {
+  case CoroutineSuspendExpr::SuspendReturnType::SuspendBool: {
     assert(SuspendRet->getType()->isIntegerTy());
 
     // Veto suspension if requested by bool returning await_suspend.
@@ -311,7 +311,7 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
     CGF.EmitBlock(RealSuspendBlock);
     break;
   }
-  case CoroutineSuspendExpr::SuspendHandle: {
+  case CoroutineSuspendExpr::SuspendReturnType::SuspendHandle: {
     assert(SuspendRet->getType()->isPointerTy());
 
     auto ResumeIntrinsic = CGF.CGM.getIntrinsic(llvm::Intrinsic::coro_resume);
diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
@@ -347,39 +347,14 @@ static Expr *maybeTailCall(Sema &S, QualType RetType, Expr *E,
 
   Expr *JustAddress = AddressExpr.get();
 
-  // FIXME: Without optimizations, the temporary result from `await_suspend()`
-  // may be put on the coroutine frame since the coroutine frame constructor
-  // will think the temporary variable will escape from the
-  // `coroutine_handle<>::address()` call. This is problematic since the
-  // coroutine should be considered to be suspended after it enters
-  // `await_suspend` so it shouldn't access/update the coroutine frame after
-  // that.
-  //
-  // See https://github.com/llvm/llvm-project/issues/65054 for the report.
-  //
-  // The long term solution may wrap the whole logic about `await-suspend`
-  // into a standalone function. This is similar to the proposed solution
-  // in tryMarkAwaitSuspendNoInline. See the comments there for details.
-  //
-  // The short term solution here is to mark `coroutine_handle<>::address()`
-  // function as always-inline so that the coroutine frame constructor won't
-  // think the temporary result is escaped incorrectly.
-  if (auto *FD = cast<CallExpr>(JustAddress)->getDirectCallee())
-    if (!FD->hasAttr<AlwaysInlineAttr>() && !FD->hasAttr<NoInlineAttr>())
-      FD->addAttr(AlwaysInlineAttr::CreateImplicit(S.getASTContext(),
-                                                   FD->getLocation()));
-
   // Check that the type of AddressExpr is void*
   if (!JustAddress->getType().getTypePtr()->isVoidPointerType())
     S.Diag(cast<CallExpr>(JustAddress)->getCalleeDecl()->getLocation(),
            diag::warn_coroutine_handle_address_invalid_return_type)
         << JustAddress->getType();
 
-  // Clean up temporary objects so that they don't live across suspension points
-  // unnecessarily. We choose to clean up before the call to
-  // __builtin_coro_resume so that the cleanup code are not inserted in-between
-  // the resume call and return instruction, which would interfere with the
-  // musttail call contract.
+  // Clean up temporary objects, because the resulting expression
+  // will become the body of await_suspend wrapper.
   return S.MaybeCreateExprWithCleanups(JustAddress);
 }
 
diff --git a/clang/test/CodeGenCoroutines/coro-simplify-suspend-point.cpp b/clang/test/CodeGenCoroutines/coro-simplify-suspend-point.cpp
diff --git a/clang/test/CodeGenCoroutines/pr56329.cpp b/clang/test/CodeGenCoroutines/pr56329.cpp
@@ -115,5 +115,9 @@ Task Outer() {
 // CHECK-NOT: _exit
 // CHECK: musttail call
 // CHECK: musttail call
+// CHECK: musttail call
 // CHECK-NEXT: ret void
+// CHECK-EMPTY:
+// CHECK-NEXT: unreachable:
+// CHECK-NEXT: unreachable
 // CHECK-NEXT: }
diff --git a/clang/test/CodeGenCoroutines/pr65054.cpp b/clang/test/CodeGenCoroutines/pr65054.cpp
@@ -1,7 +1,3 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 \
-// RUN:      -O0 -disable-llvm-passes -emit-llvm %s -o - \
-// RUN:      | FileCheck %s --check-prefix=FRONTEND
-
 // The output of O0 is highly redundant and hard to test. Also it is not good
 // limit the output of O0. So we test the optimized output from O0. The idea
 // is the optimizations shouldn't change the semantics of the program.
@@ -51,9 +47,6 @@ MyTask FooBar() {
   }
 }
 
-// FRONTEND: define{{.*}}@_ZNKSt16coroutine_handleIvE7addressEv{{.*}}#[[address_attr:[0-9]+]]
-// FRONTEND: attributes #[[address_attr]] = {{.*}}alwaysinline
-
 // CHECK-O0: define{{.*}}@_Z6FooBarv.resume
 // CHECK-O0: call{{.*}}@__await_suspend_wrapper__Z6FooBarv_await(
 // CHECK-O0-NOT: store
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
