Author: Ilya Biryukov Date: 2023-03-17T17:01:43+01:00 New Revision: 4361ba791cd6ab6e41fca4df3fd337da0c116132
URL: https://github.com/llvm/llvm-project/commit/4361ba791cd6ab6e41fca4df3fd337da0c116132 DIFF: https://github.com/llvm/llvm-project/commit/4361ba791cd6ab6e41fca4df3fd337da0c116132.diff LOG: Revert "[Coroutines] Fix premature conversion of return object" This reverts commit 54225c457a336b1609c6d064b2b606a9238a28b9. The lack of RVO causes compile errors in our code. Reverting to unblock our integrate. See D145639 for full discussion. Added: Modified: clang/include/clang/AST/StmtCXX.h clang/lib/AST/StmtCXX.cpp clang/lib/CodeGen/CGCoroutine.cpp clang/lib/Sema/SemaCoroutine.cpp clang/lib/Sema/TreeTransform.h clang/test/CodeGenCoroutines/coro-gro.cpp clang/test/CodeGenCoroutines/pr59221.cpp clang/test/SemaCXX/coroutine-no-move-ctor.cpp clang/test/SemaCXX/coroutines.cpp clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp Removed: ################################################################################ diff --git a/clang/include/clang/AST/StmtCXX.h b/clang/include/clang/AST/StmtCXX.h index 05dfac2b50c3f..2c71f86768963 100644 --- a/clang/include/clang/AST/StmtCXX.h +++ b/clang/include/clang/AST/StmtCXX.h @@ -326,7 +326,6 @@ 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. @@ -353,7 +352,6 @@ class CoroutineBodyStmt final Stmt *OnFallthrough = nullptr; Expr *Allocate = nullptr; Expr *Deallocate = nullptr; - Stmt *ResultDecl = nullptr; Expr *ReturnValue = nullptr; Stmt *ReturnStmt = nullptr; Stmt *ReturnStmtOnAllocFailure = nullptr; @@ -406,7 +404,6 @@ 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]); } diff --git a/clang/lib/AST/StmtCXX.cpp b/clang/lib/AST/StmtCXX.cpp index a3ae5392f54bc..33b0421ad1016 100644 --- a/clang/lib/AST/StmtCXX.cpp +++ b/clang/lib/AST/StmtCXX.cpp @@ -117,7 +117,6 @@ 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] = diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp index 38167cc74a7f3..9b233c1807cf1 100644 --- a/clang/lib/CodeGen/CGCoroutine.cpp +++ b/clang/lib/CodeGen/CGCoroutine.cpp @@ -467,71 +467,6 @@ 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); @@ -598,13 +533,6 @@ 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(); @@ -642,8 +570,23 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) { // promise local variable was not emitted yet. CoroId->setArgOperand(1, PromiseAddrVoidPtr); - // Now we have the promise, initialize the GRO - GroManager.EmitGroInit(); + // 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); + } EHStack.pushCleanup<CallCoroEnd>(EHCleanup); @@ -706,8 +649,12 @@ 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()) + 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); EmitStmt(Ret); + } // LLVM require the frontend to mark the coroutine. CurFn->setPresplitCoroutine(); diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp index 3df2af25f6f36..0dcfbd5281d1d 100644 --- a/clang/lib/Sema/SemaCoroutine.cpp +++ b/clang/lib/Sema/SemaCoroutine.cpp @@ -1736,7 +1736,6 @@ bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() { if (Res.isInvalid()) return false; - this->ResultDecl = Res.get(); return true; } @@ -1749,55 +1748,12 @@ bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() { return false; } - // 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()); - + StmtResult ReturnStmt = S.BuildReturnStmt(Loc, ReturnValue); 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; } diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h index e9b35f658c206..21789f96f9154 100644 --- a/clang/lib/Sema/TreeTransform.h +++ b/clang/lib/Sema/TreeTransform.h @@ -8066,12 +8066,6 @@ 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()) diff --git a/clang/test/CodeGenCoroutines/coro-gro.cpp b/clang/test/CodeGenCoroutines/coro-gro.cpp index ddcf112f0da6b..fad75c9433076 100644 --- a/clang/test/CodeGenCoroutines/coro-gro.cpp +++ b/clang/test/CodeGenCoroutines/coro-gro.cpp @@ -46,14 +46,13 @@ 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( - // CHECK: store i1 true, ptr %[[GroActive]] + // 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]] Cleanup cleanup; doSomething(); @@ -69,18 +68,7 @@ int f() { // CHECK: %[[Mem:.+]] = call ptr @llvm.coro.free( // CHECK: call void @_ZdlPv(ptr noundef %[[Mem]]) - // 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: coro.ret: // CHECK: %[[LoadRet:.+]] = load i32, ptr %[[RetVal]] // CHECK: ret i32 %[[LoadRet]] } diff --git a/clang/test/CodeGenCoroutines/pr59221.cpp b/clang/test/CodeGenCoroutines/pr59221.cpp index e0e3de559a403..ae5f6fdbdea92 100644 --- a/clang/test/CodeGenCoroutines/pr59221.cpp +++ b/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 -O1 -S -emit-llvm -o - | FileCheck %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O3 -S -emit-llvm -o - | FileCheck %s #include "Inputs/coroutine.h" diff --git a/clang/test/SemaCXX/coroutine-no-move-ctor.cpp b/clang/test/SemaCXX/coroutine-no-move-ctor.cpp index 08933f4df7a8e..824dea375ebde 100644 --- a/clang/test/SemaCXX/coroutine-no-move-ctor.cpp +++ b/clang/test/SemaCXX/coroutine-no-move-ctor.cpp @@ -15,13 +15,10 @@ class invoker { }; 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() { diff --git a/clang/test/SemaCXX/coroutines.cpp b/clang/test/SemaCXX/coroutines.cpp index 782f4b2f63333..e480c0d34593a 100644 --- a/clang/test/SemaCXX/coroutines.cpp +++ b/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 lvalue of type 'void *'}} + // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}} co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}} } diff --git a/clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp b/clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp index e96aae4fefc6b..4d52bdca7ca93 100644 --- a/clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp +++ b/clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp @@ -13,6 +13,8 @@ struct task { explicit task(promise_type& p) { throw 1; p.return_val = this; } + task(const task&) = delete; + T value; }; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
