[Coroutines] Fix premature conversion of return object

Fix #56532

Effectively, this reverts behavior introduced in https://reviews.llvm.org/D117087,
which did two things:

1. Change delayed to early conversion of return object.
2. Introduced RVO possibilities because of early conversion.

This patches fixes (1) and removes (2). I already worked on a follow up for (2)
in a separated patch. I believe it's important to split these two because if the RVO
causes any problems we can explore reverting (2) while maintaining (1).

Notes on some testcase changes:
- `pr59221.cpp` changed to `-O1` so we can check that the front-end honors
  the value checked for. Sounds like `-O3` without RVO is more likely
  to work with LLVM optimizations...
- Comment out delete members `coroutine-no-move-ctor.cpp` since behavior
  now requires copies again.

Differential Revision: https://reviews.llvm.org/D145639

Author: bcardosolopes

clang/include/clang/AST/StmtCXX.h

@@ -326,6 +326,7 @@ class CoroutineBodyStmt final
     OnFallthrough, ///< Handler for control flow falling off the body.
     Allocate,      ///< Coroutine frame memory allocation.
     Deallocate,    ///< Coroutine frame memory deallocation.
+    ResultDecl,    ///< Declaration holding the result of get_return_object.
     ReturnValue,   ///< Return value for thunk function: p.get_return_object().
     ReturnStmt,    ///< Return statement for the thunk function.
     ReturnStmtOnAllocFailure, ///< Return statement if allocation failed.
@@ -352,6 +353,7 @@ class CoroutineBodyStmt final
     Stmt *OnFallthrough = nullptr;
     Expr *Allocate = nullptr;
     Expr *Deallocate = nullptr;
+    Stmt *ResultDecl = nullptr;
     Expr *ReturnValue = nullptr;
     Stmt *ReturnStmt = nullptr;
     Stmt *ReturnStmtOnAllocFailure = nullptr;
@@ -404,6 +406,7 @@ class CoroutineBodyStmt final
   Expr *getDeallocate() const {
     return cast_or_null<Expr>(getStoredStmts()[SubStmt::Deallocate]);
   }
+  Stmt *getResultDecl() const { return getStoredStmts()[SubStmt::ResultDecl]; }
   Expr *getReturnValueInit() const {
     return cast<Expr>(getStoredStmts()[SubStmt::ReturnValue]);
   }

clang/lib/AST/StmtCXX.cpp

@@ -117,6 +117,7 @@ CoroutineBodyStmt::CoroutineBodyStmt(CoroutineBodyStmt::CtorArgs const &Args)
   SubStmts[CoroutineBodyStmt::OnFallthrough] = Args.OnFallthrough;
   SubStmts[CoroutineBodyStmt::Allocate] = Args.Allocate;
   SubStmts[CoroutineBodyStmt::Deallocate] = Args.Deallocate;
+  SubStmts[CoroutineBodyStmt::ResultDecl] = Args.ResultDecl;
   SubStmts[CoroutineBodyStmt::ReturnValue] = Args.ReturnValue;
   SubStmts[CoroutineBodyStmt::ReturnStmt] = Args.ReturnStmt;
   SubStmts[CoroutineBodyStmt::ReturnStmtOnAllocFailure] =

clang/lib/CodeGen/CGCoroutine.cpp

@@ -467,6 +467,71 @@ struct CallCoroDelete final : public EHScopeStack::Cleanup {
 };
 }
 
+namespace {
+struct GetReturnObjectManager {
+  CodeGenFunction &CGF;
+  CGBuilderTy &Builder;
+  const CoroutineBodyStmt &S;
+
+  Address GroActiveFlag;
+  CodeGenFunction::AutoVarEmission GroEmission;
+
+  GetReturnObjectManager(CodeGenFunction &CGF, const CoroutineBodyStmt &S)
+      : CGF(CGF), Builder(CGF.Builder), S(S), GroActiveFlag(Address::invalid()),
+        GroEmission(CodeGenFunction::AutoVarEmission::invalid()) {}
+
+  // The gro variable has to outlive coroutine frame and coroutine promise, but,
+  // it can only be initialized after coroutine promise was created, thus, we
+  // split its emission in two parts. EmitGroAlloca emits an alloca and sets up
+  // cleanups. Later when coroutine promise is available we initialize the gro
+  // and sets the flag that the cleanup is now active.
+  void EmitGroAlloca() {
+    auto *GroDeclStmt = dyn_cast<DeclStmt>(S.getResultDecl());
+    if (!GroDeclStmt) {
+      // If get_return_object returns void, no need to do an alloca.
+      return;
+    }
+
+    auto *GroVarDecl = cast<VarDecl>(GroDeclStmt->getSingleDecl());
+
+    // Set GRO flag that it is not initialized yet
+    GroActiveFlag = CGF.CreateTempAlloca(Builder.getInt1Ty(), CharUnits::One(),
+                                         "gro.active");
+    Builder.CreateStore(Builder.getFalse(), GroActiveFlag);
+
+    GroEmission = CGF.EmitAutoVarAlloca(*GroVarDecl);
+
+    // Remember the top of EHStack before emitting the cleanup.
+    auto old_top = CGF.EHStack.stable_begin();
+    CGF.EmitAutoVarCleanups(GroEmission);
+    auto top = CGF.EHStack.stable_begin();
+
+    // Make the cleanup conditional on gro.active
+    for (auto b = CGF.EHStack.find(top), e = CGF.EHStack.find(old_top); b != e;
+         b++) {
+      if (auto *Cleanup = dyn_cast<EHCleanupScope>(&*b)) {
+        assert(!Cleanup->hasActiveFlag() && "cleanup already has active flag?");
+        Cleanup->setActiveFlag(GroActiveFlag);
+        Cleanup->setTestFlagInEHCleanup();
+        Cleanup->setTestFlagInNormalCleanup();
+      }
+    }
+  }
+
+  void EmitGroInit() {
+    if (!GroActiveFlag.isValid()) {
+      // No Gro variable was allocated. Simply emit the call to
+      // get_return_object.
+      CGF.EmitStmt(S.getResultDecl());
+      return;
+    }
+
+    CGF.EmitAutoVarInit(GroEmission);
+    Builder.CreateStore(Builder.getTrue(), GroActiveFlag);
+  }
+};
+} // namespace
+
 static void emitBodyAndFallthrough(CodeGenFunction &CGF,
                                    const CoroutineBodyStmt &S, Stmt *Body) {
   CGF.EmitStmt(Body);
@@ -533,6 +598,13 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
       CGM.getIntrinsic(llvm::Intrinsic::coro_begin), {CoroId, Phi});
   CurCoro.Data->CoroBegin = CoroBegin;
 
+  // We need to emit `get_­return_­object` first. According to:
+  // [dcl.fct.def.coroutine]p7
+  // The call to get_­return_­object is sequenced before the call to
+  // initial_­suspend and is invoked at most once.
+  GetReturnObjectManager GroManager(*this, S);
+  GroManager.EmitGroAlloca();
+
   CurCoro.Data->CleanupJD = getJumpDestInCurrentScope(RetBB);
   {
     CGDebugInfo *DI = getDebugInfo();
@@ -570,23 +642,8 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
     // promise local variable was not emitted yet.
     CoroId->setArgOperand(1, PromiseAddrVoidPtr);
 
-    // ReturnValue should be valid as long as the coroutine's return type
-    // is not void. The assertion could help us to reduce the check later.
-    assert(ReturnValue.isValid() == (bool)S.getReturnStmt());
-    // Now we have the promise, initialize the GRO.
-    // We need to emit `get_return_object` first. According to:
-    // [dcl.fct.def.coroutine]p7
-    // The call to get_return_­object is sequenced before the call to
-    // initial_suspend and is invoked at most once.
-    //
-    // So we couldn't emit return value when we emit return statment,
-    // otherwise the call to get_return_object wouldn't be in front
-    // of initial_suspend.
-    if (ReturnValue.isValid()) {
-      EmitAnyExprToMem(S.getReturnValue(), ReturnValue,
-                       S.getReturnValue()->getType().getQualifiers(),
-                       /*IsInit*/ true);
-    }
+    // Now we have the promise, initialize the GRO
+    GroManager.EmitGroInit();
 
     EHStack.pushCleanup<CallCoroEnd>(EHCleanup);
 
@@ -649,12 +706,8 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
   llvm::Function *CoroEnd = CGM.getIntrinsic(llvm::Intrinsic::coro_end);
   Builder.CreateCall(CoroEnd, {NullPtr, Builder.getFalse()});
 
-  if (Stmt *Ret = S.getReturnStmt()) {
-    // Since we already emitted the return value above, so we shouldn't
-    // emit it again here.
-    cast<ReturnStmt>(Ret)->setRetValue(nullptr);
+  if (Stmt *Ret = S.getReturnStmt())
     EmitStmt(Ret);
-  }
 
   // LLVM require the frontend to mark the coroutine.
   CurFn->setPresplitCoroutine();

clang/lib/Sema/SemaCoroutine.cpp

@@ -1736,6 +1736,7 @@ bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() {
     if (Res.isInvalid())
       return false;
 
+    this->ResultDecl = Res.get();
     return true;
   }
 
@@ -1748,12 +1749,55 @@ bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() {
     return false;
   }
 
-  StmtResult ReturnStmt = S.BuildReturnStmt(Loc, ReturnValue);
+  // StmtResult ReturnStmt = S.BuildReturnStmt(Loc, ReturnValue);
+  auto *GroDecl = VarDecl::Create(
+      S.Context, &FD, FD.getLocation(), FD.getLocation(),
+      &S.PP.getIdentifierTable().get("__coro_gro"), GroType,
+      S.Context.getTrivialTypeSourceInfo(GroType, Loc), SC_None);
+  GroDecl->setImplicit();
+
+  S.CheckVariableDeclarationType(GroDecl);
+  if (GroDecl->isInvalidDecl())
+    return false;
+
+  InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl);
+  ExprResult Res =
+      S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
+  if (Res.isInvalid())
+    return false;
+
+  Res = S.ActOnFinishFullExpr(Res.get(), /*DiscardedValue*/ false);
+  if (Res.isInvalid())
+    return false;
+
+  S.AddInitializerToDecl(GroDecl, Res.get(),
+                         /*DirectInit=*/false);
+
+  S.FinalizeDeclaration(GroDecl);
+
+  // Form a declaration statement for the return declaration, so that AST
+  // visitors can more easily find it.
+  StmtResult GroDeclStmt =
+      S.ActOnDeclStmt(S.ConvertDeclToDeclGroup(GroDecl), Loc, Loc);
+  if (GroDeclStmt.isInvalid())
+    return false;
+
+  this->ResultDecl = GroDeclStmt.get();
+
+  ExprResult declRef = S.BuildDeclRefExpr(GroDecl, GroType, VK_LValue, Loc);
+  if (declRef.isInvalid())
+    return false;
+
+  StmtResult ReturnStmt = S.BuildReturnStmt(Loc, declRef.get());
+
   if (ReturnStmt.isInvalid()) {
     noteMemberDeclaredHere(S, ReturnValue, Fn);
     return false;
   }
 
+  if (cast<clang::ReturnStmt>(ReturnStmt.get())->getNRVOCandidate() == GroDecl)
+    GroDecl->setNRVOVariable(true);
+
   this->ReturnStmt = ReturnStmt.get();
   return true;
 }

clang/lib/Sema/TreeTransform.h

@@ -8051,6 +8051,12 @@ TreeTransform<Derived>::TransformCoroutineBodyStmt(CoroutineBodyStmt *S) {
       return StmtError();
     Builder.Deallocate = DeallocRes.get();
 
+    assert(S->getResultDecl() && "ResultDecl must already be built");
+    StmtResult ResultDecl = getDerived().TransformStmt(S->getResultDecl());
+    if (ResultDecl.isInvalid())
+      return StmtError();
+    Builder.ResultDecl = ResultDecl.get();
+
     if (auto *ReturnStmt = S->getReturnStmt()) {
       StmtResult Res = getDerived().TransformStmt(ReturnStmt);
       if (Res.isInvalid())

clang/test/CodeGenCoroutines/coro-gro.cpp

@@ -46,13 +46,14 @@ void doSomething() noexcept;
 // CHECK: define{{.*}} i32 @_Z1fv(
 int f() {
   // CHECK: %[[RetVal:.+]] = alloca i32
+  // CHECK: %[[GroActive:.+]] = alloca i1
 
   // CHECK: %[[Size:.+]] = call i64 @llvm.coro.size.i64()
   // CHECK: call noalias noundef nonnull ptr @_Znwm(i64 noundef %[[Size]])
+  // CHECK: store i1 false, ptr %[[GroActive]]
   // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_typeC1Ev(
-  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(ptr sret(%struct.GroType) align {{[0-9]+}} %[[GRO:.+]],
-  // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv({{.*}}[[GRO]]
-  // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(
+  // CHECK: store i1 true, ptr %[[GroActive]]
 
   Cleanup cleanup;
   doSomething();
@@ -68,7 +69,18 @@ int f() {
   // CHECK: %[[Mem:.+]] = call ptr @llvm.coro.free(
   // CHECK: call void @_ZdlPv(ptr noundef %[[Mem]])
 
-  // CHECK: coro.ret:
+  // Initialize retval from Gro and destroy Gro
+
+  // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv(
+  // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+  // CHECK: %[[IsActive:.+]] = load i1, ptr %[[GroActive]]
+  // CHECK: br i1 %[[IsActive]], label %[[CleanupGro:.+]], label %[[Done:.+]]
+
+  // CHECK: [[CleanupGro]]:
+  // CHECK:   call void @_ZN7GroTypeD1Ev(
+  // CHECK:   br label %[[Done]]
+
+  // CHECK: [[Done]]:
   // CHECK:   %[[LoadRet:.+]] = load i32, ptr %[[RetVal]]
   // CHECK:   ret i32 %[[LoadRet]]
 }

clang/test/CodeGenCoroutines/pr59221.cpp

@@ -2,7 +2,7 @@
 //
 // REQUIRES: x86-registered-target
 //
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O3 -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O1 -S -emit-llvm -o - | FileCheck %s
 
 #include "Inputs/coroutine.h"
 

clang/test/SemaCXX/coroutine-no-move-ctor.cpp

@@ -15,10 +15,13 @@ class invoker {
   };
   using promise_type = invoker_promise;
   invoker() {}
-  invoker(const invoker &) = delete;
-  invoker &operator=(const invoker &) = delete;
-  invoker(invoker &&) = delete;
-  invoker &operator=(invoker &&) = delete;
+  // TODO: implement RVO for get_return_object type matching
+  // function return type.
+  //
+  // invoker(const invoker &) = delete;
+  // invoker &operator=(const invoker &) = delete;
+  // invoker(invoker &&) = delete;
+  // invoker &operator=(invoker &&) = delete;
 };
 
 invoker f() {

clang/test/SemaCXX/coroutines.cpp

@@ -934,7 +934,7 @@ struct std::coroutine_traits<int, mismatch_gro_type_tag2> {
 
 extern "C" int f(mismatch_gro_type_tag2) {
   // cxx2b-error@-1 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
-  // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
+  // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an lvalue of type 'void *'}}
   co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
 }
 

clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp

@@ -13,8 +13,6 @@ struct task {
 
     explicit task(promise_type& p) { throw 1; p.return_val = this; }
 
-    task(const task&) = delete;
-
     T value;
 };