https://github.com/NewSigma updated https://github.com/llvm/llvm-project/pull/151067
>From 114dc53ea11f442f1e1cf094cbef9a877538bb67 Mon Sep 17 00:00:00 2001 From: NewSigma <[email protected]> Date: Mon, 22 Dec 2025 10:51:53 +0800 Subject: [PATCH] Resolve review comments --- clang/lib/CodeGen/CGCoroutine.cpp | 133 ++++++++++++++++++---- clang/test/CodeGenCoroutines/coro-gro.cpp | 71 ++++++++---- 2 files changed, 159 insertions(+), 45 deletions(-) diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp index b76450152203d..f103e4c06f741 100644 --- a/clang/lib/CodeGen/CGCoroutine.cpp +++ b/clang/lib/CodeGen/CGCoroutine.cpp @@ -43,6 +43,8 @@ struct clang::CodeGen::CGCoroData { // A branch to this block is emitted when coroutine needs to suspend. llvm::BasicBlock *SuspendBB = nullptr; + // A branch to this block after final.cleanup or final.ready + llvm::BasicBlock *FinalExit = nullptr; // The promise type's 'unhandled_exception' handler, if it defines one. Stmt *ExceptionHandler = nullptr; @@ -332,6 +334,8 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co // Emit cleanup for this suspend point. CGF.EmitBlock(CleanupBlock); CGF.EmitBranchThroughCleanup(Coro.CleanupJD); + if (IsFinalSuspend) + Coro.FinalExit = CleanupBlock->getSingleSuccessor(); // Emit await_resume expression. CGF.EmitBlock(ReadyBlock); @@ -687,11 +691,11 @@ struct GetReturnObjectManager { } // 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() { + // it can only be initialized after coroutine promise was created. Thus, + // EmitGroActive emits a flag and sets it to false. Later when coroutine + // promise is available we initialize the gro and set the flag indicating that + // the cleanup is now active. + void EmitGroActive() { if (DirectEmit) return; @@ -701,12 +705,23 @@ struct GetReturnObjectManager { 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); + } + + void EmitGroAlloca() { + 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()); GroEmission = CGF.EmitAutoVarAlloca(*GroVarDecl); @@ -768,6 +783,78 @@ struct GetReturnObjectManager { CGF.EmitAutoVarInit(GroEmission); Builder.CreateStore(Builder.getTrue(), GroActiveFlag); } + // The GRO returns either when it is first suspended or when it completes + // without ever being suspended. The EmitGroConv function evaluates these + // conditions and perform the conversion if needed. + // + // Before EmitGroConv(): + // final.exit: + // switch i32 %cleanup.dest, label %destroy [ + // i32 0, label %after.ready + // ] + // + // after.ready: + // ; (empty) + // + // After EmitGroConv(): + // final.exit: + // switch i32 %cleanup.dest, label %destroy [ + // i32 0, label %pre.gro.conv + // ] + // + // pre.gro.conv: + // %IsFinalExit = phi i1 [ false, %any.suspend ], [ true, %final.exit ] + // %InRamp = call i1 @llvm.coro.is_in_ramp() + // br i1 %InRamp, label %gro.conv, label %after.gro.conv + // + // gro.conv: + // ; GRO conversion + // br label %after.gro.conv + // + // after.gro.conv: + // br i1 %IsFinalExit, label %after.ready, label %coro.ret + void EmitGroConv(BasicBlock *RetBB) { + auto *AfterReadyBB = Builder.GetInsertBlock(); + Builder.ClearInsertionPoint(); + + auto *PreConvBB = CGF.CurCoro.Data->SuspendBB; + CGF.EmitBlock(PreConvBB); + // If final.exit exists, redirect it to PreConvBB + llvm::PHINode *IsFinalExit = nullptr; + if (BasicBlock *FinalExit = CGF.CurCoro.Data->FinalExit) { + assert(AfterReadyBB && + AfterReadyBB->getSinglePredecessor() == FinalExit && + "Expect fallthrough from final.exit block"); + AfterReadyBB->replaceAllUsesWith(PreConvBB); + PreConvBB->moveBefore(AfterReadyBB); + + // If true, coroutine completes and should be destroyed after conversion + IsFinalExit = + Builder.CreatePHI(Builder.getInt1Ty(), llvm::pred_size(PreConvBB)); + for (auto *Pred : llvm::predecessors(PreConvBB)) { + auto *V = (Pred == FinalExit) ? Builder.getTrue() : Builder.getFalse(); + IsFinalExit->addIncoming(V, Pred); + } + } + auto *InRampFn = CGF.CGM.getIntrinsic(llvm::Intrinsic::coro_is_in_ramp); + auto *InRamp = Builder.CreateCall(InRampFn, {}, "InRamp"); + auto *ConvBB = CGF.createBasicBlock("gro.conv"); + auto *AfterConvBB = CGF.createBasicBlock("after.gro.conv"); + Builder.CreateCondBr(InRamp, ConvBB, AfterConvBB); + + CGF.EmitBlock(ConvBB); + CGF.EmitAnyExprToMem(S.getReturnValue(), CGF.ReturnValue, + S.getReturnValue()->getType().getQualifiers(), + /*IsInit*/ true); + Builder.CreateBr(AfterConvBB); + + CGF.EmitBlock(AfterConvBB); + if (IsFinalExit) + Builder.CreateCondBr(IsFinalExit, AfterReadyBB, RetBB); + else + Builder.CreateBr(RetBB); + Builder.SetInsertPoint(AfterReadyBB); + } }; } // namespace @@ -795,7 +882,10 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) { CGM.getIntrinsic(llvm::Intrinsic::coro_id), {Builder.getInt32(NewAlign), NullPtr, NullPtr, NullPtr}); createCoroData(*this, CurCoro, CoroId); - CurCoro.Data->SuspendBB = RetBB; + + GetReturnObjectManager GroManager(*this, S); + CurCoro.Data->SuspendBB = + GroManager.DirectEmit ? RetBB : createBasicBlock("pre.gvo.conv"); assert(ShouldEmitLifetimeMarkers && "Must emit lifetime intrinsics for coroutines"); @@ -839,9 +929,6 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) { CGM.getIntrinsic(llvm::Intrinsic::coro_begin), {CoroId, Phi}); CurCoro.Data->CoroBegin = CoroBegin; - GetReturnObjectManager GroManager(*this, S); - GroManager.EmitGroAlloca(); - CurCoro.Data->CleanupJD = getJumpDestInCurrentScope(RetBB); { CGDebugInfo *DI = getDebugInfo(); @@ -884,6 +971,7 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) { // not needed. } + GroManager.EmitGroActive(); EmitStmt(S.getPromiseDeclStmt()); Address PromiseAddr = GetAddrOfLocalVar(S.getPromiseDecl()); @@ -895,6 +983,7 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) { CoroId->setArgOperand(1, PromiseAddrVoidPtr); // Now we have the promise, initialize the GRO + GroManager.EmitGroAlloca(); GroManager.EmitGroInit(); EHStack.pushCleanup<CallCoroEnd>(EHCleanup); @@ -950,31 +1039,31 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) { // We don't need FinalBB. Emit it to make sure the block is deleted. EmitBlock(FinalBB, /*IsFinished=*/true); } + + // We need conversion if get_return_object's type doesn't matches the + // coroutine return type. + if (!GroManager.DirectEmit) + GroManager.EmitGroConv(RetBB); } EmitBlock(RetBB); - // Emit coro.end before getReturnStmt (and parameter destructors), since - // resume and destroy parts of the coroutine should not include them. + // Emit coro.end before ret instruction, since resume and destroy parts of the + // coroutine should return void. llvm::Function *CoroEnd = CGM.getIntrinsic(llvm::Intrinsic::coro_end); Builder.CreateCall(CoroEnd, {NullPtr, Builder.getFalse(), llvm::ConstantTokenNone::get(CoroEnd->getContext())}); - if (Stmt *Ret = S.getReturnStmt()) { + if (auto *Ret = cast_or_null<ReturnStmt>(S.getReturnStmt())) { // Since we already emitted the return value above, so we shouldn't // emit it again here. - Expr *PreviousRetValue = nullptr; - if (GroManager.DirectEmit) { - PreviousRetValue = cast<ReturnStmt>(Ret)->getRetValue(); - cast<ReturnStmt>(Ret)->setRetValue(nullptr); - } + Expr *PreviousRetValue = Ret->getRetValue(); + Ret->setRetValue(nullptr); EmitStmt(Ret); // Set the return value back. The code generator, as the AST **Consumer**, // shouldn't change the AST. - if (PreviousRetValue) - cast<ReturnStmt>(Ret)->setRetValue(PreviousRetValue); + Ret->setRetValue(PreviousRetValue); } - // LLVM require the frontend to mark the coroutine. CurFn->setPresplitCoroutine(); diff --git a/clang/test/CodeGenCoroutines/coro-gro.cpp b/clang/test/CodeGenCoroutines/coro-gro.cpp index 037fd03349e76..fb9d0a6d85377 100644 --- a/clang/test/CodeGenCoroutines/coro-gro.cpp +++ b/clang/test/CodeGenCoroutines/coro-gro.cpp @@ -29,47 +29,72 @@ void doSomething() noexcept; // CHECK: define{{.*}} i32 @_Z1fv( int f() { // CHECK: %[[RetVal:.+]] = alloca i32 - // CHECK: %[[GroActive:.+]] = alloca i1 - // CHECK: %[[CoroGro:.+]] = alloca %struct.GroType, {{.*}} !coro.outside.frame ![[OutFrameMetadata:.+]] + // CHECK-NEXT: %[[GroActive:.+]] = alloca i1 + // CHECK-NEXT: %[[Promise:.+]] = alloca %"struct.std::coroutine_traits<int>::promise_type", align 1 + // CHECK-NEXT: %[[CoroGro:.+]] = alloca %struct.GroType, {{.*}} !coro.outside.frame ![[OutFrameMetadata:.+]] // CHECK: %[[Size:.+]] = call i64 @llvm.coro.size.i64() - // CHECK: call noalias noundef nonnull ptr @_Znwm(i64 noundef %[[Size]]) + // CHECK-NEXT: call noalias noundef nonnull ptr @_Znwm(i64 noundef %[[Size]]) + // CHECK: store i1 false, ptr %[[GroActive]] - // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_typeC1Ev( - // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_type17get_return_objectEv({{.*}} %[[CoroGro]] - // CHECK: store i1 true, ptr %[[GroActive]] + // CHECK-NEXT: call void @llvm.lifetime.start.p0(ptr %[[Promise]]) + // CHECK-NEXT: call void @_ZNSt16coroutine_traitsIiJEE12promise_typeC1Ev( + // CHECK-NEXT: call void @llvm.lifetime.start.p0(ptr %[[CoroGro]]) + // CHECK-NEXT: call void @_ZNSt16coroutine_traitsIiJEE12promise_type17get_return_objectEv({{.*}} %[[CoroGro]] + // CHECK-NEXT: store i1 true, ptr %[[GroActive]] Cleanup cleanup; doSomething(); co_return; // CHECK: call void @_Z11doSomethingv( - // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_type11return_voidEv( - // CHECK: call void @_ZN7CleanupD1Ev( - - // Destroy promise and free the memory. - - // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_typeD1Ev( - // CHECK: %[[Mem:.+]] = call ptr @llvm.coro.free( - // CHECK: %[[SIZE:.+]] = call i64 @llvm.coro.size.i64() - // CHECK: call void @_ZdlPvm(ptr noundef %[[Mem]], i64 noundef %[[SIZE]]) + // CHECK-NEXT: call void @_ZNSt16coroutine_traitsIiJEE12promise_type11return_voidEv( + // CHECK-NEXT: call void @_ZN7CleanupD1Ev( // 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: pre.gvo.conv: + // CHECK-NEXT: %10 = phi i1 [ true, %cleanup8 ], [ false, %final.suspend ], [ false, %init.suspend ] + // CHECK-NEXT: %InRamp = call i1 @llvm.coro.is_in_ramp() + // CHECK-NEXT: br i1 %InRamp, label %[[GroConv:.+]], label %[[AfterGroConv:.+]] + + // CHECK: [[GroConv]]: + // CHECK-NEXT: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv( + // CHECK-NEXT: store i32 %[[Conv]], ptr %[[RetVal]] + // CHECK-NEXT: br label %[[AfterGroConv]] + + // CHECK: [[AfterGroConv]]: + // CHECK-NEXT: br i1 %10, label %cleanup.cont10, label %[[CoroRet:.+]] + + // CHECK: cleanup.cont10: + // CHECK-NEXT: br label %[[Cleanup:.+]] + + // CHECK: [[Cleanup]]: + // CHECK-NEXT: %{{.*}} = phi i32 + // CHECK-NEXT: %[[IsActive:.+]] = load i1, ptr %[[GroActive]] + // CHECK-NEXT: br i1 %[[IsActive]], label %[[CleanupGro:.+]], label %[[Done:.+]] // CHECK: [[CleanupGro]]: - // CHECK: call void @_ZN7GroTypeD1Ev( - // CHECK: br label %[[Done]] + // CHECK-NEXT: call void @_ZN7GroTypeD1Ev( + // CHECK-NEXT: br label %[[Done]] + + // Destroy promise and free the memory. // CHECK: [[Done]]: - // CHECK: %[[LoadRet:.+]] = load i32, ptr %[[RetVal]] - // CHECK: ret i32 %[[LoadRet]] + // CHECK-NEXT: call void @llvm.lifetime.end.p0(ptr %[[CoroGro]]) + // CHECK-NEXT: call void @_ZNSt16coroutine_traitsIiJEE12promise_typeD1Ev( + // CHECK-NEXT: call void @llvm.lifetime.end.p0(ptr %[[Promise]]) + // CHECK-NEXT: %[[Mem:.+]] = call ptr @llvm.coro.free( + + // CHECK: %[[SIZE:.+]] = call i64 @llvm.coro.size.i64() + // CHECK-NEXT: call void @_ZdlPvm(ptr noundef %[[Mem]], i64 noundef %[[SIZE]]) + + // CHECK: [[CoroRet]]: + // CHECK-NEXT: call void @llvm.coro.end( + // CHECK-NEXT: %[[LoadRet:.+]] = load i32, ptr %[[RetVal]] + // CHECK-NEXT: ret i32 %[[LoadRet]] } class invoker { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
