llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Morris Hafner (mmha) <details> <summary>Changes</summary> * A coroutine may be destroyed after its `initial_suspend`. Add an edge from there to `destroy` * `suspend` has no edge to `resume`. Add this, too --- Full diff: https://github.com/llvm/llvm-project/pull/230466.diff 3 Files Affected: - (modified) clang/include/clang/CIR/Dialect/IR/CIROps.td (+14-13) - (modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+14-4) - (modified) clang/unittests/CIR/ControlFlowTest.cpp (+16-6) ``````````diff diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index a0d19941a5c5be..dca57f8eba9a36 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -4889,11 +4889,13 @@ def CIR_CoroutineOp : CIR_Op<"coroutine", [ points and introduce structured control flow that transfers execution to `final_suspend`. - - `final_suspend`: exactly one `cir.await`, marked `final`. Nothing - else. Corresponds to `co_await promise.final_suspend()`. Resuming - past this point is undefined behavior in valid programs, so its - `resume` edge is expected to be dead in practice, but is kept for - structural symmetry with the other two suspend regions. + - `final_suspend`: at most one `cir.await`, marked `final`. Nothing + else. Corresponds to `co_await promise.final_suspend()`. Suspending + at this final suspend point is valid: the coroutine stays suspended + until it is destroyed. Resuming past this point is undefined behavior + in valid programs, so its `resume` edge is expected to be dead in + practice, but is kept for structural symmetry with the other two + suspend regions. - `gro`: TODO. @@ -4901,10 +4903,10 @@ def CIR_CoroutineOp : CIR_Op<"coroutine", [ - `destroy`: destroys the coroutine frame (`coro.free` + `delete`). Reached by an explicit `destroy()` call on a suspended handle, by - normal completion through `final_suspend`, or by an exception - escaping the coroutine. Falls through to `exit` except when reached - due to an escaping exception, where it continues propagating that - exception instead. + normal completion through `initial_suspend`, `final_suspend`, or + by an exception escaping the coroutine. Falls through to `exit` + except when reached due to an escaping exception, where it continues + propagating that exception instead. - `exit`: the shared return-to-caller path. Reached directly from any suspend point that isn't being destroyed (the coroutine is just @@ -5048,10 +5050,9 @@ def CIR_CoReturnOp : CIR_Op<"co_return", [ ]> { let summary = "Coroutine return operation"; let description = [{ - The `cir.co_return` operation models a coroutine return point inside a - `cir.coro.body` region. - This operation is expected to appear only within a `cir.coro.body` region, - but it may be nested within other operations or regions inside that body. + The `cir.co_return` operation models a coroutine return point inside the + `body` region of a `cir.coroutine`. It may be nested within other + operations or regions inside that body. }]; let assemblyFormat = [{ diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp index a51ab0f1ed3de4..7e427da1000147 100644 --- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp @@ -3680,7 +3680,15 @@ void cir::AwaitOp::getSuccessorRegions( return; } - // Branching from suspend or resume: exit to the parent operation. + // Branching from suspend: continue in resume once the coroutine is resumed, + // or exit to the parent operation if it stays suspended or is destroyed. + if (&getSuspend() == parentRegion) { + regions.emplace_back(&getResume()); + regions.emplace_back(getOperation()); + return; + } + + // Branching from resume: exit to the parent operation. regions.emplace_back(getOperation()); } @@ -3765,11 +3773,13 @@ void cir::CoroutineOp::getSuccessorRegions( point.getTerminatorPredecessorOrNull()->getParentRegion(); // initial_suspend either falls into body (resumed, or never actually - // suspended because await_ready() was true) or exits directly, a plain - // suspend here means nobody has resumed yet, so we just return to caller. + // suspended because await_ready() was true), exits directly, or reaches + // destroy when the coroutine is destroyed while suspended at its initial + // suspend point. if (parent == &getInitialSuspend()) { regions.emplace_back(&getBody()); regions.emplace_back(&getExit()); + regions.emplace_back(&getDestroy()); return; } @@ -3784,7 +3794,7 @@ void cir::CoroutineOp::getSuccessorRegions( return; } - // final_suspend's only live edge in valid programs is destroy resuming + // final_suspend's only live edge in valid programs is destroy. Resuming // past the final suspend is UB. The ready-immediately edge to exit is // kept for structural symmetry with the other two suspend regions even // though it's effectively dead. diff --git a/clang/unittests/CIR/ControlFlowTest.cpp b/clang/unittests/CIR/ControlFlowTest.cpp index 51b22c620c0a17..236f481eef8f47 100644 --- a/clang/unittests/CIR/ControlFlowTest.cpp +++ b/clang/unittests/CIR/ControlFlowTest.cpp @@ -648,10 +648,12 @@ TEST_F(CIRControlFlowTest, CoroutineOp) { RegionBranchTerminatorOpInterface initTerm = getTerminator(coroOp.getInitialSuspend()); ASSERT_TRUE(initTerm); - expectSuccessors(coroOp, RegionBranchPoint(initTerm), - {&coroOp.getBody(), &coroOp.getExit()}); - expectTerminatorSuccessors(coroOp.getInitialSuspend(), - {&coroOp.getBody(), &coroOp.getExit()}); + expectSuccessors( + coroOp, RegionBranchPoint(initTerm), + {&coroOp.getBody(), &coroOp.getExit(), &coroOp.getDestroy()}); + expectTerminatorSuccessors( + coroOp.getInitialSuspend(), + {&coroOp.getBody(), &coroOp.getExit(), &coroOp.getDestroy()}); // body: falls through to final_suspend, exits directly on a plain // suspend, or reaches destroy @@ -741,10 +743,18 @@ TEST_F(CIRControlFlowTest, AwaitOp) { expectTerminatorSuccessors(awaitOp.getReady(), {&awaitOp.getResume(), &awaitOp.getSuspend()}); - expectTerminatorSuccessors(awaitOp.getSuspend(), {nullptr}); + expectTerminatorSuccessors(awaitOp.getSuspend(), + {&awaitOp.getResume(), nullptr}); expectTerminatorSuccessors(awaitOp.getResume(), {nullptr}); - EXPECT_FALSE(asRegionBranch(awaitOp).hasLoop()); + // MLIR's RegionBranchOpInterface::hasLoop() reports any region reached + // twice as a loop, and resume is reachable both directly from ready and + // through suspend. + RegionBranchOpInterface awaitBranch = asRegionBranch(awaitOp); + EXPECT_FALSE(awaitBranch.isRepetitiveRegion(0)); + EXPECT_FALSE(awaitBranch.isRepetitiveRegion(1)); + EXPECT_FALSE(awaitBranch.isRepetitiveRegion(2)); + EXPECT_TRUE(awaitBranch.hasLoop()); verifyControlFlowInterfaceConsistency(awaitOp); } `````````` </details> https://github.com/llvm/llvm-project/pull/230466 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
