[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

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

Add a newline.

[bruno]

Apply last round of reviews before landing

clang/docs/ReleaseNotes.rst

- Fix crash when parsing fold expression containing a delayed typo correction.
   (`#61326 <https://github.com/llvm/llvm-project/issues/61326>`_)
 - Fix crash when dealing with some member accesses outside of class or member
   function context.
   (`#37792 <https://github.com/llvm/llvm-project/issues/37792>`_) and
   (`#48405 <https://github.com/llvm/llvm-project/issues/48405>`_)
 - Fix crash when using ``[[clang::always_inline]]`` or ``[[clang::noinline]]``
   statement attributes on a call to a template function in the body of a
   template function.
+- Fix coroutines issue where ``get_return_object()`` result was always eargerly
+  converted to the return type. Eager initialization (allowing RVO) is now only
+  perfomed when these types match, otherwise deferred initialization is used,
+  enabling short-circuiting coroutines use cases. This fixes
+  (`#56532 <https://github.com/llvm/llvm-project/issues/56532>`_) in
+  antecipation of `CWG2563 <https://cplusplus.github.io/CWG/issues/2563.html>_`.
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

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();
+      assert(RVI && "expected RVI");
+      auto GroType = RVI->getType();
+      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;
   }
 
-  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/CodeGenCoroutines/coro-gro.cpp

 // Verifies lifetime of __gro local variable
 // Verify that coroutine promise and allocated memory are freed up on exception.
 // RUN: %clang_cc1 -std=c++20 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s -disable-llvm-passes | FileCheck %s
 
-namespace std {
-template <typename... T> struct coroutine_traits;
+#include "Inputs/coroutine.h"
 
-template <class Promise = void> struct coroutine_handle {
-  coroutine_handle() = default;
-  static coroutine_handle from_address(void *) noexcept;
-};
-template <> struct coroutine_handle<void> {
-  static coroutine_handle from_address(void *) noexcept;
-  coroutine_handle() = default;
-  template <class PromiseType>
-  coroutine_handle(coroutine_handle<PromiseType>) noexcept;
-};
-} // namespace std
-
-struct suspend_always {
-  bool await_ready() noexcept;
-  void await_suspend(std::coroutine_handle<>) noexcept;
-  void await_resume() noexcept;
-};
+using namespace std;
 
 struct GroType {
   ~GroType();
   operator int() noexcept;
 };
 
 template <> struct std::coroutine_traits<int> {
   struct promise_type {

 // 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(
+  // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_typeC1Ev(
+  // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_type17get_return_objectEv(
   // CHECK: store i1 true, ptr %[[GroActive]]
 
   Cleanup cleanup;
   doSomething();
   co_return;
 
   // CHECK: call void @_Z11doSomethingv(
-  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type11return_voidEv(
+  // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_type11return_voidEv(
   // CHECK: call void @_ZN7CleanupD1Ev(
 
   // Destroy promise and free the memory.
 
-  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_typeD1Ev(
+  // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_typeD1Ev(
   // CHECK: %[[Mem:.+]] = call ptr @llvm.coro.free(
   // CHECK: call void @_ZdlPv(ptr noundef %[[Mem]])
 
   // Initialize retval from Gro and destroy Gro
+  // Note this also tests delaying initialization when Gro and function return
+  // types mismatch (see cwg2563).
 
   // 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]]
 }
+
+class invoker {
+public:
+  class invoker_promise {
+  public:
+    invoker get_return_object() { return invoker{}; }
+    auto initial_suspend() { return suspend_always{}; }
+    auto final_suspend() noexcept { return suspend_always{}; }
+    void return_void() {}
+    void unhandled_exception() {}
+  };
+  using promise_type = invoker_promise;
+  invoker() {}
+  invoker(const invoker &) = delete;
+  invoker &operator=(const invoker &) = delete;
+  invoker(invoker &&) = delete;
+  invoker &operator=(invoker &&) = delete;
+};
+
+// According to cwg2563, matching GRO and function return type must allow
+// for eager initialization and RVO.
+// CHECK: define{{.*}} void @_Z1gv({{.*}} %[[AggRes:.+]])
+invoker g() {
+  // CHECK: %[[ResultPtr:.+]] = alloca ptr
+  // CHECK-NEXT: %[[Promise:.+]] = alloca %"class.invoker::invoker_promise"
+
+  // CHECK: store ptr %[[AggRes]], ptr %[[ResultPtr]]
+  // CHECK: coro.init:
+  // CHECK: = call ptr @llvm.coro.begin
+
+  // delayed GRO pattern stores a GRO active flag, make sure to not emit it.
+  // CHECK-NOT: store i1 false, ptr
+  // CHECK: call void @_ZN7invoker15invoker_promise17get_return_objectEv({{.*}} %[[AggRes]]
+  co_return;
+}

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;
 }