Author: Henrich Lauko Date: 2026-10-15T08:19:03+02:00 New Revision: 16b2fbe109b0cb8822c931167aa43fb441e79c04
URL: https://github.com/llvm/llvm-project/commit/16b2fbe109b0cb8822c931167aa43fb441e79c04 DIFF: https://github.com/llvm/llvm-project/commit/16b2fbe109b0cb8822c931167aa43fb441e79c04.diff LOG: [CIR] Fix cir.cleanup.scope successor regions (#221222) `CleanupScopeOp::getSuccessorRegions` listed the body and the cleanup region as entries from the parent, so the cleanup could run without the body and no edge led from the body to the cleanup. Now the parent enters the body, a body exit enters the cleanup region if the cleanup kind covers that exit, and the cleanup region exits to the parent. `cir.resume` runs EH cleanups, and `cir.yield` and `cir.co_return` run normal ones. Take this scope, where `<kind>` is `normal`, `eh` or `all`: ```mlir cir.cleanup.scope { cir.brcond %cond ^normal, ^unwind ^normal: cir.yield ^unwind: %tok = cir.eh.initiate cleanup : !cir.eh_token cir.resume %tok : !cir.eh_token } cleanup <kind> { cir.yield } ``` `getSuccessorRegions` reports these successors for each branch point: | Branch point | Before this patch, any kind | After, `cleanup normal` | After, `cleanup eh` | After, `cleanup all` | |---|---|---|---|---| | entering the op | body or cleanup | body | body | body | | body `cir.yield` | leave the op | cleanup | leave the op | cleanup | | body `cir.resume` | leave the op | leave the op | cleanup | cleanup | | cleanup `cir.yield` | leave the op | leave the op | leave the op | leave the op | The interface only describes exits through a region terminator. A throw from a call inside the body is not one, so it gets no edge: ```mlir cir.cleanup.scope { cir.call @may_throw() : () -> () // a throw here runs the cleanup cir.yield // leaves the op } cleanup eh { cir.yield } ``` Here no edge reaches the cleanup region. Likewise, a `cir.return`, `cir.break`, `cir.continue` or `cir.goto` out of the body runs a normal cleanup but has no edge to it. Assisted-by: Claude Code (Claude Opus 5.5). Added: Modified: clang/include/clang/CIR/Dialect/IR/CIROps.td clang/lib/CIR/Dialect/IR/CIRDialect.cpp clang/unittests/CIR/ControlFlowTest.cpp Removed: ################################################################################ diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index 48fbc158f60daa..121e4666d5eb50 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -1458,12 +1458,11 @@ def CIR_CleanupScopeOp : CIR_RegionBranchOpBase<"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 67903fc8c75320..f564eee0537057 100644 --- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp @@ -2045,14 +2045,31 @@ LogicalResult cir::CleanupScopeOp::verify() { 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 unwinds out of the body. Every other terminator, such as + // cir.yield or cir.co_return, is a normal exit. + 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 is not a + // RegionBranchPoint, and neither are cir.return, cir.break, cir.continue or + // cir.goto, so a cleanup region reached only that way has no predecessor + // here. + regions.emplace_back(getOperation()); } LogicalResult cir::CleanupScopeOp::canonicalize(CleanupScopeOp op, diff --git a/clang/unittests/CIR/ControlFlowTest.cpp b/clang/unittests/CIR/ControlFlowTest.cpp index 236f481eef8f47..e3a6d46ffdbbba 100644 --- a/clang/unittests/CIR/ControlFlowTest.cpp +++ b/clang/unittests/CIR/ControlFlowTest.cpp @@ -80,53 +80,86 @@ static void expectTerminatorSuccessors(Region ®ion, } /// Verify control flow interface consistency beyond what mlir::verify checks: -/// - Every non-empty region is reachable -/// - Every terminator implements RegionBranchTerminatorOpInterface -/// - Op and terminator agree on successor regions -static void verifyControlFlowInterfaceConsistency(RegionBranchOpInterface op) { +/// - Every non-empty region is reachable, unless `requireAllReachable` is +/// false +/// - Every non-empty region has at least one branching terminator +/// - Op and terminator agree on successor regions at every branch point +static void +verifyControlFlowInterfaceConsistency(RegionBranchOpInterface op, + bool requireAllReachable = true) { EXPECT_TRUE(succeeded(mlir::verify(op))) << "MLIR verifier failed for '" << op->getName().getStringRef().str() << "'"; - SmallVector<RegionSuccessor> entrySuccessors; - op.getSuccessorRegions(RegionBranchPoint::parent(), entrySuccessors); - llvm::SmallPtrSet<Region *, 4> allReachable; - for (auto &succ : entrySuccessors) - allReachable.insert(succ.getSuccessor()); - for (Region ®ion : op->getRegions()) { - if (region.empty()) - continue; - RegionBranchTerminatorOpInterface term = getTerminator(region); + // Walk the same branch points the verifier uses: the op itself plus one per + // branching block terminator. Going block by block rather than through + // region.front() lets a region exit diff erently from diff erent blocks. + llvm::SmallPtrSet<Region *, 4> allReachable, regionsWithExit; + for (RegionBranchPoint point : op.getAllRegionBranchPoints()) { SmallVector<RegionSuccessor> opSuccessors; - op.getSuccessorRegions(region, opSuccessors); - for (auto &succ : opSuccessors) + op.getSuccessorRegions(point, opSuccessors); + for (RegionSuccessor &succ : opSuccessors) allReachable.insert(succ.getSuccessor()); + if (point.isParent()) + continue; + // Op and terminator should report the same successors. - if (term) { - SmallVector<RegionSuccessor> termSuccessors; - term.getSuccessorRegions(/*operands=*/{}, termSuccessors); - SmallPtrSet<Region *, 4> opSet, termSet; - for (auto &s : opSuccessors) - opSet.insert(s.getSuccessor()); - for (auto &s : termSuccessors) - termSet.insert(s.getSuccessor()); - EXPECT_EQ(opSet, termSet) - << "op and terminator disagree on successors from region #" - << region.getRegionNumber(); - } + RegionBranchTerminatorOpInterface term = + point.getTerminatorPredecessorOrNull(); + regionsWithExit.insert(term->getParentRegion()); + SmallVector<RegionSuccessor> termSuccessors; + term.getSuccessorRegions(/*operands=*/{}, termSuccessors); + SmallPtrSet<Region *, 4> opSet, termSet; + for (RegionSuccessor &s : opSuccessors) + opSet.insert(s.getSuccessor()); + for (RegionSuccessor &s : termSuccessors) + termSet.insert(s.getSuccessor()); + EXPECT_EQ(opSet, termSet) + << "op and terminator disagree on successors from region #" + << term->getParentRegion()->getRegionNumber(); } + for (Region ®ion : op->getRegions()) { if (region.empty()) continue; - EXPECT_TRUE(allReachable.contains(®ion)) + EXPECT_TRUE(!requireAllReachable || allReachable.contains(®ion)) << "Region #" << region.getRegionNumber() << " is non-empty but not reachable from any branch point"; + EXPECT_TRUE(regionsWithExit.contains(®ion)) + << "Region #" << region.getRegionNumber() + << " has no RegionBranchTerminatorOpInterface terminator"; } EXPECT_TRUE(allReachable.contains(nullptr)) << "parent (exit) not reachable from any branch point"; } +/// Check a cir.cleanup.scope whose body has a single exit. The parent enters +/// the body, the body exit enters the cleanup region if `bodyRunsCleanup` and +/// leaves the op otherwise, and the cleanup region leaves the op. +static void expectCleanupScopeSuccessors(cir::CleanupScopeOp op, + bool bodyRunsCleanup) { + Region &body = op.getBodyRegion(); + Region &cleanup = op.getCleanupRegion(); + expectSuccessors(op, RegionBranchPoint::parent(), {&body}); + + RegionBranchTerminatorOpInterface bodyTerm = getTerminator(body); + ASSERT_TRUE(bodyTerm); + expectSuccessors(op, RegionBranchPoint(bodyTerm), + {bodyRunsCleanup ? &cleanup : nullptr}); + expectTerminatorSuccessors(cleanup, {nullptr}); + + RegionBranchOpInterface branch = op; + EXPECT_FALSE(branch.isRepetitiveRegion(0)); + EXPECT_FALSE(branch.isRepetitiveRegion(1)); + EXPECT_FALSE(branch.hasLoop()); + + // Without the body edge nothing reaches the cleanup region. + verifyControlFlowInterfaceConsistency( + op, + /*requireAllReachable=*/bodyRunsCleanup); +} + //===----------------------------------------------------------------------===// // Test fixture //===----------------------------------------------------------------------===// @@ -513,7 +546,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 { @@ -524,19 +557,111 @@ TEST_F(CIRControlFlowTest, CleanupScopeOp) { cir.return } )CIR"); + // A cleanup that covers normal exits runs on the body's cir.yield. + expectCleanupScopeSuccessors(findFirstOp<cir::CleanupScopeOp>(*module), + /*bodyRunsCleanup=*/true); +} + +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"); + // An EH-only cleanup is skipped by the body's normal cir.yield. + expectCleanupScopeSuccessors(findFirstOp<cir::CleanupScopeOp>(*module), + /*bodyRunsCleanup=*/false); +} + +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"); + // An EH-only cleanup runs on the body's unwinding cir.resume. + expectCleanupScopeSuccessors(findFirstOp<cir::CleanupScopeOp>(*module), + /*bodyRunsCleanup=*/true); +} + +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"); + // A normal-only cleanup is skipped by the body's unwinding cir.resume. + expectCleanupScopeSuccessors(findFirstOp<cir::CleanupScopeOp>(*module), + /*bodyRunsCleanup=*/false); +} + +TEST_F(CIRControlFlowTest, CleanupScopeOpMultiBlockBodyNormal) { + // One block leaves the body normally and another unwinds, which is what + // FlattenCFG produces once an inner cleanup scope has been flattened. Under + // `cleanup normal` the two exits disagree, so this is the case that needs a + // branch point per block rather than one per region. + OwningOpRef<ModuleOp> module = parse(R"CIR( + cir.func @f() { + cir.cleanup.scope { + %cond = cir.const #cir.bool<true> : !cir.bool + cir.brcond %cond ^bb_normal, ^bb_unwind + ^bb_normal: + cir.yield + ^bb_unwind: + %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); - expectSuccessors( - cleanupScopeOp, RegionBranchPoint::parent(), - {&cleanupScopeOp.getBodyRegion(), &cleanupScopeOp.getCleanupRegion()}); - expectTerminatorSuccessors(cleanupScopeOp.getBodyRegion(), {nullptr}); + expectSuccessors(cleanupScopeOp, RegionBranchPoint::parent(), + {&cleanupScopeOp.getBodyRegion()}); + + // The cir.brcond block is not a branch point, so only the two exiting + // blocks show up here. + SmallVector<RegionBranchTerminatorOpInterface> bodyTerms; + for (RegionBranchPoint point : + asRegionBranch(cleanupScopeOp).getAllRegionBranchPoints()) + if (!point.isParent() && + point.getTerminatorPredecessorOrNull()->getParentRegion() == + &cleanupScopeOp.getBodyRegion()) + bodyTerms.push_back(point.getTerminatorPredecessorOrNull()); + ASSERT_EQ(bodyTerms.size(), 2u); + EXPECT_TRUE(isa<cir::YieldOp>(*bodyTerms[0])); + EXPECT_TRUE(isa<cir::ResumeOp>(*bodyTerms[1])); + + // The normal exit runs the cleanup, the unwind exit skips it. + expectSuccessors(cleanupScopeOp, RegionBranchPoint(bodyTerms[0]), + {&cleanupScopeOp.getCleanupRegion()}); + expectSuccessors(cleanupScopeOp, RegionBranchPoint(bodyTerms[1]), {nullptr}); expectTerminatorSuccessors(cleanupScopeOp.getCleanupRegion(), {nullptr}); - RegionBranchOpInterface cleanupBranch = asRegionBranch(cleanupScopeOp); - EXPECT_FALSE(cleanupBranch.isRepetitiveRegion(0)); - EXPECT_FALSE(cleanupBranch.isRepetitiveRegion(1)); - EXPECT_FALSE(cleanupBranch.hasLoop()); + EXPECT_FALSE(asRegionBranch(cleanupScopeOp).hasLoop()); + // The cleanup region does have a predecessor here, via the normal exit. verifyControlFlowInterfaceConsistency(cleanupScopeOp); } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
