https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/221222
>From b54287d64c4b96bb6e46765c5e41418b2a4f054f 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 Follow-up to #220507, split out of the review of #220512. `CleanupScopeOp::getSuccessorRegions` reported the body and the cleanup region as two alternative entries from the parent, and every region exit as going back to the parent. That lets the cleanup run without the body and leaves no edge from the body to the cleanup. The comment right above the code said the opposite. This reports what the op actually executes. The parent enters the body. A body exit that triggers the cleanup enters the cleanup region. The cleanup region exits to the parent. Whether a body exit triggers the cleanup depends on the terminator and the cleanup kind. `cir.resume` leaves while unwinding, so it runs the cleanup when the kind includes EH. `cir.yield` and `cir.co_return` are normal exits, so they run it when the kind includes normal. `cir.resume` appears in an unflattened body once an inner cleanup scope has been flattened, and `FlattenCFG` handles that case in `collectResumeOps` under the same condition. `RegionBranchOpInterface` can't describe an unwind from the middle of the body, only one from a terminator. A cleanup region reachable only that way ends up with no predecessor. No edge carries values. The op has no results and `NoRegionArguments`, and `ResumeOp::getMutableSuccessorOperands` forwards nothing, so `detail::verifyRegionBranchOpInterface` sees zero operands and inputs on every edge. The unit test checks all four pairs of exit terminator and cleanup kind with one helper, `expectCleanupScopeSuccessors`. For the two pairs that leave the cleanup region without a predecessor, it calls `verifyControlFlowInterfaceConsistency` with the new `requireAllReachable` flag set to false. A body can exit differently from different blocks, so `verifyControlFlowInterfaceConsistency` now iterates over `getAllRegionBranchPoints()` instead of each region's `front()` terminator, as `detail::verifyRegionBranchOpInterface` does. A new test relies on this for a `cleanup normal` body that yields from one block and resumes from another. Also drop the `none` cleanup kind, which `CIR_CleanupKind` never defined, from the op description, along with the claim that the cleanup runs on every body exit. The last raw cleanup-kind enum comparisons in `FlattenCFG` move to the same `isNormal()` and `isEH()` predicates in #221234. That PR is independent and can land in either order. --- clang/include/clang/CIR/Dialect/IR/CIROps.td | 9 +- clang/lib/CIR/Dialect/IR/CIRDialect.cpp | 26 ++- clang/unittests/CIR/ControlFlowTest.cpp | 199 +++++++++++++++---- 3 files changed, 187 insertions(+), 47 deletions(-) diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index aa32ada090f91..debe973dd1f9a 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -1438,12 +1438,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 0fb9f585d2228..8f24bc5a8eb33 100644 --- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp @@ -2037,14 +2037,30 @@ 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 and a cir.return are + // not RegionBranchPoints, 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 236f481eef8f4..e3a6d46ffdbbb 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 differently from different 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
