[Coroutines] Implement fix for cwg2563 issue and enable RVO under certain conditions

[bruno]

Summary

• The cwg2563 issue is fixed by delaying GRO initialization only when the types mismatch between GRO and function return.
• When the types match, directly initialize, enabling RVO to kick in as introduced in https://reviews.llvm.org/D117087

Background:
https://github.com/llvm/llvm-project/issues/56532
https://cplusplus.github.io/CWG/issues/2563.html
https://github.com/cplusplus/papers/issues/1414

Depends on D145639

[ChuanqiXu]

clang/lib/CodeGen/CGCoroutine.cpp

475

nit. Rewords it if you like.

484–500

I'll feel better if we can attach the conclusion and links from the wg21.

501–509

What's the case about returning void and the return_object type is different from the returning type?

clang/lib/Sema/SemaCoroutine.cpp

1733

Let's have some comments here.

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

24–26

How about adding a failing case that the types are not matched?

[bruno]

clang/lib/CodeGen/CGCoroutine.cpp

484–500

I can add some of that but unfortunately we don't have the working yet, it still has a CWG round to go: https://github.com/cplusplus/papers/issues/1414

The best explanation for the problem is under Gor's writeup: https://github.com/GorNishanov/await/blob/master/2023_Issaquah/cwg2563-response.md, I instead added a comment based on it since we cannot guarantee this link will be there forever.

501–509

In that case we do DirectEmit since there's no reason to delay initialization, am I missing something?

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

24–26

I'm having trouble crafting one. I extracted similar examples from other testcases and even though I can confirm they correctly apply delayed initialization, they don't require copies (usually directly call conversion operators or similar) and we don't trigger the reverse usecase, do you have one testcase in mind I could use?

[bruno]

Update after reviewer comments

[ChuanqiXu]

clang/lib/CodeGen/CGCoroutine.cpp

501–509

I mean it looks invalid if the function returns void and the return_object_type returns a non-void type. So we can judge this by hasSameType() directly.

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

24–26

Oh, I don't have an existing test. Although not the best, maybe you can try to emit the codes to LLVM IR and match that.

[bruno]

Address last round of review

[bruno]

Add release notes entry.

[ChuanqiXu]

LGTM. Thanks.

clang/docs/ReleaseNotes.rst

222–223 ↗

(On Diff #507087)

I feel it would slightly better to claim that the RVO is till performed if the return type matches with the type of get_return_object(). So that the users won't get worries.

clang/test/CodeGenCoroutines/coro-gro.cpp

107 ↗

(On Diff #507087)

Add a newline.

[bruno]

Apply last round of reviews before landing

clang/include/clang/AST/StmtCXX.h

public:
   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]);
   }
   Expr *getReturnValue() const {
-    assert(getReturnStmt());
-    auto *RS = cast<clang::ReturnStmt>(getReturnStmt());
-    return RS->getRetValue();
+    auto *RS = dyn_cast_or_null<clang::ReturnStmt>(getReturnStmt());
+    return RS ? RS->getRetValue() : nullptr;
   }
   Stmt *getReturnStmt() const { return getStoredStmts()[SubStmt::ReturnStmt]; }
   Stmt *getReturnStmtOnAllocFailure() const {
     return getStoredStmts()[SubStmt::ReturnStmtOnAllocFailure];
   }
   ArrayRef<Stmt const *> getParamMoves() const {
     return {getStoredStmts() + SubStmt::FirstParamMove, NumParams};
   }

clang/lib/CodeGen/CGCoroutine.cpp

 };
 }
 
 namespace {
 struct GetReturnObjectManager {
   CodeGenFunction &CGF;
   CGBuilderTy &Builder;
   const CoroutineBodyStmt &S;
+  // When true, performs RVO for the return object.
+  bool DirectEmit = false;
 
   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()) {}
+        GroEmission(CodeGenFunction::AutoVarEmission::invalid()) {
+    // The call to get_­return_­object is sequenced before the call to
+    // initial_­suspend and is invoked at most once, but there are caveats
+    // regarding on whether the prvalue result object may be initialized
+    // directly/eager or delayed, depending on the types involved.
+    //
+    // More info at https://github.com/cplusplus/papers/issues/1414
+    //
+    // The general cases:
+    // 1. Same type of get_return_object and coroutine return type (direct
+    // emission):
+    //  - Constructed in the return slot.
+    // 2. Different types (delayed emission):
+    //  - Constructed temporary object prior to initial suspend initialized with
+    //  a call to get_return_object()
+    //  - When coroutine needs to to return to the caller and needs to construct
+    //  return value for the coroutine it is initialized with expiring value of
+    //  the temporary obtained above.
+    //
+    // Direct emission for void returning coroutines or GROs.
+    DirectEmit = [&]() {
+      auto *RVI = S.getReturnValueInit();
+      if (!RVI || CGF.FnRetTy->isVoidType())
+        return true;
+      auto GroType = RVI->getType();
+      if (GroType->isVoidType())
+        return true;
+      return CGF.getContext().hasSameType(GroType, CGF.FnRetTy);
+    }();
+  }
 
   // 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 (DirectEmit)
+      return;
+
+    auto *GroDeclStmt = dyn_cast_or_null<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
for (auto b = CGF.EHStack.find(top), e = CGF.EHStack.find(old_top); b != e;
         Cleanup->setActiveFlag(GroActiveFlag);
         Cleanup->setTestFlagInEHCleanup();
         Cleanup->setTestFlagInNormalCleanup();
       }
     }
   }
 
   void EmitGroInit() {
+    if (DirectEmit) {
+      // 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(CGF.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 (CGF.ReturnValue.isValid()) {
+        CGF.EmitAnyExprToMem(S.getReturnValue(), CGF.ReturnValue,
+                             S.getReturnValue()->getType().getQualifiers(),
+                             /*IsInit*/ true);
+      }
+      return;
+    }
+
     if (!GroActiveFlag.isValid()) {
       // No Gro variable was allocated. Simply emit the call to
       // get_return_object.
       CGF.EmitStmt(S.getResultDecl());
       return;
     }
 
     CGF.EmitAutoVarInit(GroEmission);
void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
   // Pass the result of the allocation to coro.begin.
   auto *Phi = Builder.CreatePHI(VoidPtrTy, 2);
   Phi->addIncoming(NullPtr, EntryBB);
   Phi->addIncoming(AllocateCall, AllocOrInvokeContBB);
   auto *CoroBegin = Builder.CreateCall(
       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();
     ParamReferenceReplacerRAII ParamReplacer(LocalDeclMap);
     CodeGenFunction::RunCleanupsScope ResumeScope(*this);
void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
   }
 
   EmitBlock(RetBB);
   // Emit coro.end before getReturnStmt (and parameter destructors), since
   // resume and destroy parts of the coroutine should not include them.
   llvm::Function *CoroEnd = CGM.getIntrinsic(llvm::Intrinsic::coro_end);
   Builder.CreateCall(CoroEnd, {NullPtr, Builder.getFalse()});
 
-  if (Stmt *Ret = S.getReturnStmt())
+  if (Stmt *Ret = S.getReturnStmt()) {
+    // Since we already emitted the return value above, so we shouldn't
+    // emit it again here.
+    if (GroManager.DirectEmit)
+      cast<ReturnStmt>(Ret)->setRetValue(nullptr);
     EmitStmt(Ret);
+  }
 
   // LLVM require the frontend to mark the coroutine.
   CurFn->setPresplitCoroutine();
 }
 
 // Emit coroutine intrinsic and patch up arguments of the token type.
 RValue CodeGenFunction::EmitCoroutineIntrinsic(const CallExpr *E,
                                                unsigned int IID) {

clang/lib/Sema/SemaCoroutine.cpp

bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() {
   QualType const GroType = this->ReturnValue->getType();
   assert(!GroType->isDependentType() &&
          "get_return_object type must no longer be dependent");
 
   QualType const FnRetType = FD.getReturnType();
   assert(!FnRetType->isDependentType() &&
          "get_return_object type must no longer be dependent");
 
+  // The call to get_­return_­object is sequenced before the call to
+  // initial_­suspend and is invoked at most once, but there are caveats
+  // regarding on whether the prvalue result object may be initialized
+  // directly/eager or delayed, depending on the types involved.
+  //
+  // More info at https://github.com/cplusplus/papers/issues/1414
+  bool GroMatchesRetType = S.getASTContext().hasSameType(GroType, FnRetType);
+
   if (FnRetType->isVoidType()) {
     ExprResult Res =
         S.ActOnFinishFullExpr(this->ReturnValue, Loc, /*DiscardedValue*/ false);
     if (Res.isInvalid())
       return false;
 
+    if (!GroMatchesRetType)
       this->ResultDecl = Res.get();
     return true;
   }
 
   if (GroType->isVoidType()) {
     // Trigger a nice error message.
     InitializedEntity Entity =
         InitializedEntity::InitializeResult(Loc, FnRetType);
     S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
     noteMemberDeclaredHere(S, ReturnValue, Fn);
     return false;
   }
 
-  // StmtResult ReturnStmt = S.BuildReturnStmt(Loc, ReturnValue);
-  auto *GroDecl = VarDecl::Create(
+  StmtResult ReturnStmt;
+  clang::VarDecl *GroDecl = nullptr;
+  if (GroMatchesRetType) {
+    ReturnStmt = S.BuildReturnStmt(Loc, ReturnValue);
+  } else {
+    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());
+    ReturnStmt = S.BuildReturnStmt(Loc, declRef.get());
+  }
 
   if (ReturnStmt.isInvalid()) {
     noteMemberDeclaredHere(S, ReturnValue, Fn);
     return false;
   }
 
-  if (cast<clang::ReturnStmt>(ReturnStmt.get())->getNRVOCandidate() == GroDecl)
+  if (!GroMatchesRetType &&
+      cast<clang::ReturnStmt>(ReturnStmt.get())->getNRVOCandidate() == GroDecl)
     GroDecl->setNRVOVariable(true);
 
   this->ReturnStmt = ReturnStmt.get();
   return true;
 }
 
 // Create a static_cast\<T&&>(expr).
 static Expr *castForMoving(Sema &S, Expr *E, QualType T = QualType()) {

clang/lib/Sema/TreeTransform.h

if (AllocRes.isInvalid())
       return StmtError();
     Builder.Allocate = AllocRes.get();
 
     ExprResult DeallocRes = getDerived().TransformExpr(S->getDeallocate());
     if (DeallocRes.isInvalid())
       return StmtError();
     Builder.Deallocate = DeallocRes.get();
 
-    assert(S->getResultDecl() && "ResultDecl must already be built");
-    StmtResult ResultDecl = getDerived().TransformStmt(S->getResultDecl());
-    if (ResultDecl.isInvalid())
+    if (auto *ResultDecl = S->getResultDecl()) {
+      StmtResult Res = getDerived().TransformStmt(ResultDecl);
+      if (Res.isInvalid())
         return StmtError();
-    Builder.ResultDecl = ResultDecl.get();
+      Builder.ResultDecl = Res.get();
+    }
 
     if (auto *ReturnStmt = S->getReturnStmt()) {
       StmtResult Res = getDerived().TransformStmt(ReturnStmt);
       if (Res.isInvalid())
         return StmtError();
       Builder.ReturnStmt = Res.get();
     }
   }

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

public:
     invoker get_return_object() { return invoker{}; }
     auto initial_suspend() { return std::suspend_never{}; }
     auto final_suspend() noexcept { return std::suspend_never{}; }
     void return_void() {}
     void unhandled_exception() {}
   };
   using promise_type = invoker_promise;
   invoker() {}
-  // 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(const invoker &) = delete;
+  invoker &operator=(const invoker &) = delete;
+  invoker(invoker &&) = delete;
+  invoker &operator=(invoker &&) = delete;
 };
 
 invoker f() {
   co_return;
 }