https://github.com/xlauko created https://github.com/llvm/llvm-project/pull/221222
This is a follow-up to #220507, split out of the review discussion on #220512. `CleanupScopeOp::getSuccessorRegions` reported both the body and the cleanup region as successors of the parent operation, and the parent operation as the successor of every region exit. That claims the cleanup region can be entered without running the body, and it drops the body-to-cleanup edge entirely, contradicting the comment that sat right above it. This reports the chain the operation actually executes: the body region is entered from the operation, an exit from the body that triggers the cleanup runs the cleanup region, and exiting the cleanup region leaves the operation. Which body exits trigger the cleanup depends on both the terminator and the cleanup kind. `cir.resume` leaves the body while unwinding, so it runs the cleanup exactly when the kind is EH-capable; `cir.yield` and `cir.co_return` are normal exits and run it when the kind covers normal exits. A `cir.resume` can terminate a block of an unflattened cleanup-scope body once an inner cleanup scope has been flattened, which is the case `FlattenCFG` handles in `collectResumeOps` under the same condition. Unwinding from an arbitrary point inside the body rather than from one of its terminators remains outside what `RegionBranchOpInterface` can describe, so a cleanup region whose only trigger is that kind of unwind has no predecessor. `cir.cleanup.scope` declares no results and carries `NoRegionArguments`, and `ResumeOp::getMutableSuccessorOperands` forwards none of its operands, so successor operand and input counts stay at zero along every edge and `detail::verifyRegionBranchOpInterface` is unaffected. The unit test covers the four combinations of exit terminator and cleanup kind. The two whose cleanup region ends up without a predecessor cannot use `verifyControlFlowInterfaceConsistency`, which requires every non-empty region to be reachable from some branch point. This also drops the `none` cleanup kind from the op description, which `CIR_CleanupKind` never defined, and stops claiming the cleanup region runs on every exit from the body. >From 0f6ba55d24a7af84bb4e6e621897241c33eddbdf Mon Sep 17 00:00:00 2001 From: Henrich Lauko <[email protected]> Date: Fri, 4 Sep 2026 11:20:11 +0000 Subject: [PATCH] [CIR] Fix cir.cleanup.scope successor regions CleanupScopeOp::getSuccessorRegions reported both the body and the cleanup region as successors of the parent operation, and the parent operation as the successor of every region exit. That claims the cleanup region can be entered without running the body, and it drops the body-to-cleanup edge entirely, contradicting the comment that sat right above it. Report the chain the operation actually executes: the body region is entered from the operation, an exit from the body that triggers the cleanup runs the cleanup region, and exiting the cleanup region leaves the operation. Which body exits trigger the cleanup depends on both the terminator and the cleanup kind. cir.resume leaves the body while unwinding, so it runs the cleanup exactly when the kind is EH-capable; cir.yield and cir.co_return are normal exits and run it when the kind covers normal exits. A cir.resume can terminate a block of an unflattened cleanup-scope body once an inner cleanup scope has been flattened, which is the case FlattenCFG handles in collectResumeOps under the same condition. Unwinding from an arbitrary point inside the body rather than from one of its terminators remains outside what RegionBranchOpInterface can describe, so a cleanup region whose only trigger is that kind of unwind has no predecessor. cir.cleanup.scope declares no results and carries NoRegionArguments, and ResumeOp::getMutableSuccessorOperands forwards none of its operands, so successor operand and input counts stay at zero along every edge and detail::verifyRegionBranchOpInterface is unaffected. Cover the four combinations of exit terminator and cleanup kind in the unit test. The two whose cleanup region ends up without a predecessor cannot use verifyControlFlowInterfaceConsistency, which requires every non-empty region to be reachable from some branch point. Also drop the `none` cleanup kind from the op description, which CIR_CleanupKind never defined, and stop claiming the cleanup region runs on every exit from the body. --- clang/include/clang/CIR/Dialect/IR/CIROps.td | 7 +- clang/lib/CIR/Dialect/IR/CIRDialect.cpp | 30 +++++- clang/unittests/CIR/ControlFlowTest.cpp | 101 ++++++++++++++++++- 3 files changed, 125 insertions(+), 13 deletions(-) diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index 5c2c948742f08..932b27d611862 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -1382,11 +1382,10 @@ def CIR_CleanupScopeOp : CIR_Op<"cleanup.scope", [ let description = [{ `cir.cleanup.scope` contains a body region and a cleanup region. The body region is executed first, and the cleanup region is executed when the body - region is exited, either normally or due to an exception. + region is exited. - The cleanup kind attribute specifies when the cleanup region should be - executed: - - `none`: No cleanup (cleanup region is empty/unused) + The cleanup kind attribute specifies which exits from the body region run + the cleanup region: - `normal`: Cleanup is executed only on normal exit - `eh`: Cleanup is executed only on exception unwinding - `all`: Cleanup is executed on both normal exit and exception unwinding diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp index c3838413984a6..a665420dc6941 100644 --- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp @@ -1692,14 +1692,34 @@ LogicalResult cir::ScopeOp::fold(FoldAdaptor /*adaptor*/, void cir::CleanupScopeOp::getSuccessorRegions( mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) { - if (!point.isParent()) { - regions.emplace_back(getOperation()); + assert(point.isParent() || point.getTerminatorPredecessorOrNull()); + + // Execution always starts in the body region. + if (point.isParent()) { + regions.emplace_back(&getBodyRegion()); return; } - // Execution always proceeds from the body region to the cleanup region. - regions.push_back(RegionSuccessor(&getBodyRegion())); - regions.push_back(RegionSuccessor(&getCleanupRegion())); + mlir::Operation *term = point.getTerminatorPredecessorOrNull(); + + if (term->getParentRegion() == &getBodyRegion()) { + // cir.resume leaves the body while unwinding, so it runs the cleanup + // region exactly when the cleanup is EH-capable. Every other branch + // terminator (cir.yield, cir.co_return) is a normal exit and runs the + // cleanup when it triggers on normal exits. + cir::CleanupKindAttr kind = getCleanupKindAttr(); + bool runsCleanup = isa<cir::ResumeOp>(term) ? kind.isEH() : kind.isNormal(); + if (runsCleanup) { + regions.emplace_back(&getCleanupRegion()); + return; + } + } + + // Exiting the cleanup region, or leaving the body along an exit that skips + // the cleanup, leaves the operation. Unwinding from a point inside the body + // rather than from one of its terminators is not structured control flow and + // cannot be reported here at all. + regions.emplace_back(getOperation()); } mlir::ValueRange diff --git a/clang/unittests/CIR/ControlFlowTest.cpp b/clang/unittests/CIR/ControlFlowTest.cpp index d6b9fda5235e6..686709124573d 100644 --- a/clang/unittests/CIR/ControlFlowTest.cpp +++ b/clang/unittests/CIR/ControlFlowTest.cpp @@ -513,7 +513,7 @@ TEST_F(CIRControlFlowTest, ForOpWithCleanup) { verifyControlFlowInterfaceConsistency(forOp); } -TEST_F(CIRControlFlowTest, CleanupScopeOp) { +TEST_F(CIRControlFlowTest, CleanupScopeOpNormal) { OwningOpRef<ModuleOp> module = parse(R"CIR( cir.func @f() { cir.cleanup.scope { @@ -526,9 +526,40 @@ TEST_F(CIRControlFlowTest, CleanupScopeOp) { )CIR"); auto cleanupScopeOp = findFirstOp<cir::CleanupScopeOp>(*module); - expectSuccessors( - cleanupScopeOp, RegionBranchPoint::parent(), - {&cleanupScopeOp.getBodyRegion(), &cleanupScopeOp.getCleanupRegion()}); + // A cleanup that runs on normal exits forms a chain: the body is entered + // from the operation, exiting the body runs the cleanup, and exiting the + // cleanup leaves the operation. + expectSuccessors(cleanupScopeOp, RegionBranchPoint::parent(), + {&cleanupScopeOp.getBodyRegion()}); + expectTerminatorSuccessors(cleanupScopeOp.getBodyRegion(), + {&cleanupScopeOp.getCleanupRegion()}); + expectTerminatorSuccessors(cleanupScopeOp.getCleanupRegion(), {nullptr}); + + RegionBranchOpInterface cleanupBranch = asRegionBranch(cleanupScopeOp); + EXPECT_FALSE(cleanupBranch.isRepetitiveRegion(0)); + EXPECT_FALSE(cleanupBranch.isRepetitiveRegion(1)); + EXPECT_FALSE(cleanupBranch.hasLoop()); + + verifyControlFlowInterfaceConsistency(cleanupScopeOp); +} + +TEST_F(CIRControlFlowTest, CleanupScopeOpEHOnly) { + OwningOpRef<ModuleOp> module = parse(R"CIR( + cir.func @f() { + cir.cleanup.scope { + cir.yield + } cleanup eh { + cir.yield + } + cir.return + } + )CIR"); + auto cleanupScopeOp = findFirstOp<cir::CleanupScopeOp>(*module); + + // An EH-only cleanup is skipped by normal exits from the body, so the body + // leaves the operation directly. + expectSuccessors(cleanupScopeOp, RegionBranchPoint::parent(), + {&cleanupScopeOp.getBodyRegion()}); expectTerminatorSuccessors(cleanupScopeOp.getBodyRegion(), {nullptr}); expectTerminatorSuccessors(cleanupScopeOp.getCleanupRegion(), {nullptr}); @@ -537,9 +568,71 @@ TEST_F(CIRControlFlowTest, CleanupScopeOp) { EXPECT_FALSE(cleanupBranch.isRepetitiveRegion(1)); EXPECT_FALSE(cleanupBranch.hasLoop()); + // The cleanup region is only entered while unwinding, and this body unwinds + // from an arbitrary point inside itself rather than from a terminator, which + // is not an edge RegionBranchOpInterface can describe. So the cleanup region + // has no predecessor. verifyControlFlowInterfaceConsistency doesn't apply: + // it requires every non-empty region to be reachable. + EXPECT_TRUE(succeeded(mlir::verify(cleanupScopeOp))); +} + +TEST_F(CIRControlFlowTest, CleanupScopeOpResumeEH) { + // A cir.resume left behind in the body by an already-flattened inner + // cleanup scope exits the body while unwinding. + OwningOpRef<ModuleOp> module = parse(R"CIR( + cir.func @f() { + cir.cleanup.scope { + %tok = cir.eh.initiate cleanup : !cir.eh_token + cir.resume %tok : !cir.eh_token + } cleanup eh { + cir.yield + } + cir.return + } + )CIR"); + auto cleanupScopeOp = findFirstOp<cir::CleanupScopeOp>(*module); + + // An EH-only cleanup runs on the unwind exit, so this body does reach the + // cleanup region. + expectSuccessors(cleanupScopeOp, RegionBranchPoint::parent(), + {&cleanupScopeOp.getBodyRegion()}); + expectTerminatorSuccessors(cleanupScopeOp.getBodyRegion(), + {&cleanupScopeOp.getCleanupRegion()}); + expectTerminatorSuccessors(cleanupScopeOp.getCleanupRegion(), {nullptr}); + + EXPECT_FALSE(asRegionBranch(cleanupScopeOp).hasLoop()); + verifyControlFlowInterfaceConsistency(cleanupScopeOp); } +TEST_F(CIRControlFlowTest, CleanupScopeOpResumeNormalOnly) { + OwningOpRef<ModuleOp> module = parse(R"CIR( + cir.func @f() { + cir.cleanup.scope { + %tok = cir.eh.initiate cleanup : !cir.eh_token + cir.resume %tok : !cir.eh_token + } cleanup normal { + cir.yield + } + cir.return + } + )CIR"); + auto cleanupScopeOp = findFirstOp<cir::CleanupScopeOp>(*module); + + // A normal-only cleanup is skipped while unwinding, so the unwind exit + // leaves the operation without running it. + expectSuccessors(cleanupScopeOp, RegionBranchPoint::parent(), + {&cleanupScopeOp.getBodyRegion()}); + expectTerminatorSuccessors(cleanupScopeOp.getBodyRegion(), {nullptr}); + expectTerminatorSuccessors(cleanupScopeOp.getCleanupRegion(), {nullptr}); + + EXPECT_FALSE(asRegionBranch(cleanupScopeOp).hasLoop()); + + // This body has no normal exit to run the cleanup region, so it has no + // predecessor and verifyControlFlowInterfaceConsistency doesn't apply. + EXPECT_TRUE(succeeded(mlir::verify(cleanupScopeOp))); +} + TEST_F(CIRControlFlowTest, GlobalOpWithCtorAndDtor) { OwningOpRef<ModuleOp> module = parse(R"CIR( !s32i = !cir.int<s, 32> _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
