https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/221222
>From 865ab4a0f5ba1e6621a5cae9aa6ebcf5f7bd0569 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 listed both the body and the cleanup region as successors of the parent op, and the parent op as the successor of every region exit. That says the cleanup region can run without the body, and it drops the body-to-cleanup edge, contradicting the comment directly above it. Report the chain the op really executes. The parent enters the body, a body exit that triggers the cleanup enters the cleanup region, and the cleanup region exits to the parent. Which body exits trigger the cleanup depends on the terminator as well as the cleanup kind. cir.resume leaves the body while unwinding, so it runs the cleanup 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, the same case FlattenCFG handles in collectResumeOps. RegionBranchOpInterface can only describe an unwind that leaves from a terminator, not one from an arbitrary point inside the body. A cleanup region reachable only that way has no predecessor. cir.cleanup.scope has no results and carries NoRegionArguments, and ResumeOp::getMutableSuccessorOperands forwards none of its operands. Successor operand and input counts stay at zero on every edge, so detail::verifyRegionBranchOpInterface is unaffected. The unit test covers all four combinations of exit terminator and cleanup kind, each named after the pair it parses. The two that leave the cleanup region without a predecessor cannot use verifyControlFlowInterfaceConsistency, which requires every non-empty region to be reachable. Also drop the `none` cleanup kind from the op description, which CIR_CleanupKind never defined, and stop claiming the cleanup region runs on every body exit. --- clang/include/clang/CIR/Dialect/IR/CIROps.td | 9 +- clang/lib/CIR/Dialect/IR/CIRDialect.cpp | 28 ++++- clang/unittests/CIR/ControlFlowTest.cpp | 117 ++++++++++++++++++- 3 files changed, 139 insertions(+), 15 deletions(-) diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index 5c2c948742f08..254c197d2acd5 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -1381,12 +1381,11 @@ def CIR_CleanupScopeOp : CIR_Op<"cleanup.scope", [ let summary = "Represents a scope with associated cleanup code"; 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 runs first, and the cleanup region may run when the body 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..e75d077c97a9f 100644 --- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp @@ -1692,14 +1692,32 @@ LogicalResult cir::ScopeOp::fold(FoldAdaptor /*adaptor*/, void cir::CleanupScopeOp::getSuccessorRegions( mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) { - if (!point.isParent()) { - regions.emplace_back(getOperation()); + // 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(); + assert(term && "expected a terminator predecessor"); + + 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(); + if (isa<cir::ResumeOp>(term) ? kind.isEH() : kind.isNormal()) { + regions.emplace_back(&getCleanupRegion()); + return; + } + } + + // Exiting the cleanup region, or a body exit that skips it, leaves the + // operation. An unwind out of the middle of the body and a cir.return are + // not RegionBranchPoints, so a cleanup region reached only that way has no + // predecessor here. + regions.emplace_back(getOperation()); } mlir::ValueRange diff --git a/clang/unittests/CIR/ControlFlowTest.cpp b/clang/unittests/CIR/ControlFlowTest.cpp index d6b9fda5235e6..beef96a59d5a9 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, CleanupScopeOpYieldAll) { OwningOpRef<ModuleOp> module = parse(R"CIR( cir.func @f() { cir.cleanup.scope { @@ -526,10 +526,17 @@ TEST_F(CIRControlFlowTest, CleanupScopeOp) { )CIR"); auto cleanupScopeOp = findFirstOp<cir::CleanupScopeOp>(*module); - expectSuccessors( - cleanupScopeOp, RegionBranchPoint::parent(), - {&cleanupScopeOp.getBodyRegion(), &cleanupScopeOp.getCleanupRegion()}); - expectTerminatorSuccessors(cleanupScopeOp.getBodyRegion(), {nullptr}); + // A cleanup that covers normal exits runs on the body's cir.yield. + expectSuccessors(cleanupScopeOp, RegionBranchPoint::parent(), + {&cleanupScopeOp.getBodyRegion()}); + + RegionBranchTerminatorOpInterface bodyTerm = + getTerminator(cleanupScopeOp.getBodyRegion()); + ASSERT_TRUE(bodyTerm); + expectSuccessors(cleanupScopeOp, RegionBranchPoint(bodyTerm), + {&cleanupScopeOp.getCleanupRegion()}); + expectTerminatorSuccessors(cleanupScopeOp.getBodyRegion(), + {&cleanupScopeOp.getCleanupRegion()}); expectTerminatorSuccessors(cleanupScopeOp.getCleanupRegion(), {nullptr}); RegionBranchOpInterface cleanupBranch = asRegionBranch(cleanupScopeOp); @@ -540,6 +547,106 @@ TEST_F(CIRControlFlowTest, CleanupScopeOp) { verifyControlFlowInterfaceConsistency(cleanupScopeOp); } +TEST_F(CIRControlFlowTest, CleanupScopeOpYieldEH) { + 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, so the body's cir.yield + // leaves the operation directly. + expectSuccessors(cleanupScopeOp, RegionBranchPoint::parent(), + {&cleanupScopeOp.getBodyRegion()}); + + RegionBranchTerminatorOpInterface bodyTerm = + getTerminator(cleanupScopeOp.getBodyRegion()); + ASSERT_TRUE(bodyTerm); + expectSuccessors(cleanupScopeOp, RegionBranchPoint(bodyTerm), {nullptr}); + expectTerminatorSuccessors(cleanupScopeOp.getBodyRegion(), {nullptr}); + expectTerminatorSuccessors(cleanupScopeOp.getCleanupRegion(), {nullptr}); + + EXPECT_FALSE(asRegionBranch(cleanupScopeOp).hasLoop()); + + // Nothing in this body can throw and an EH-only cleanup has no normal-exit + // edge, so the cleanup region has no predecessor. That rules out + // verifyControlFlowInterfaceConsistency, which requires every non-empty + // region to be reachable. +} + +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()}); + + RegionBranchTerminatorOpInterface bodyTerm = + getTerminator(cleanupScopeOp.getBodyRegion()); + ASSERT_TRUE(bodyTerm); + expectSuccessors(cleanupScopeOp, RegionBranchPoint(bodyTerm), + {&cleanupScopeOp.getCleanupRegion()}); + expectTerminatorSuccessors(cleanupScopeOp.getBodyRegion(), + {&cleanupScopeOp.getCleanupRegion()}); + expectTerminatorSuccessors(cleanupScopeOp.getCleanupRegion(), {nullptr}); + + EXPECT_FALSE(asRegionBranch(cleanupScopeOp).hasLoop()); + + verifyControlFlowInterfaceConsistency(cleanupScopeOp); +} + +TEST_F(CIRControlFlowTest, CleanupScopeOpResumeNormal) { + 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()}); + + RegionBranchTerminatorOpInterface bodyTerm = + getTerminator(cleanupScopeOp.getBodyRegion()); + ASSERT_TRUE(bodyTerm); + expectSuccessors(cleanupScopeOp, RegionBranchPoint(bodyTerm), {nullptr}); + 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. +} + 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
