From: Ilya Biryukov Date: Fri, 17 Mar 2023 15:52:17 +0000 (+0100) Subject: Revert "[Coroutines] Fix premature conversion of return object" X-Git-Tag: upstream/17.0.6~14439 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=4361ba791cd6ab6e41fca4df3fd337da0c116132;p=platform%2Fupstream%2Fllvm.git 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. --- diff --git a/clang/include/clang/AST/StmtCXX.h b/clang/include/clang/AST/StmtCXX.h index 05dfac2..2c71f86 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 @@ public: 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 @@ public: Expr *getDeallocate() const { return cast_or_null(getStoredStmts()[SubStmt::Deallocate]); } - Stmt *getResultDecl() const { return getStoredStmts()[SubStmt::ResultDecl]; } Expr *getReturnValueInit() const { return cast(getStoredStmts()[SubStmt::ReturnValue]); } diff --git a/clang/lib/AST/StmtCXX.cpp b/clang/lib/AST/StmtCXX.cpp index a3ae539..33b0421 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 38167cc..9b233c1 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(S.getResultDecl()); - if (!GroDeclStmt) { - // If get_return_object returns void, no need to do an alloca. - return; - } - - auto *GroVarDecl = cast(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(&*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(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(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 3df2af25..0dcfbd5 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(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 e9b35f6..21789f9 100644 --- a/clang/lib/Sema/TreeTransform.h +++ b/clang/lib/Sema/TreeTransform.h @@ -8066,12 +8066,6 @@ TreeTransform::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 ddcf112..fad75c9 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 e0e3de5..ae5f6fd 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 08933f4..824dea3 100644 --- a/clang/test/SemaCXX/coroutine-no-move-ctor.cpp +++ b/clang/test/SemaCXX/coroutine-no-move-ctor.cpp @@ -15,13 +15,10 @@ public: }; 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 782f4b2..e480c0d 100644 --- a/clang/test/SemaCXX/coroutines.cpp +++ b/clang/test/SemaCXX/coroutines.cpp @@ -934,7 +934,7 @@ struct std::coroutine_traits { 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 e96aae4..4d52bdc 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; };