https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/221222
>From 0abd69204a6a7a8b74813183ae19c70878fa5b20 Mon Sep 17 00:00:00 2001 From: Henrich Lauko <[email protected]> Date: Fri, 4 Sep 2026 11:20:11 +0000 Subject: [PATCH 1/2] [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. A body can also exit differently from different blocks, so verifyControlFlowInterfaceConsistency now walks getAllRegionBranchPoints() rather than each region's front() terminator, and compares the op against the terminator once per branch point. That is what detail::verifyRegionBranchOpInterface does, and it lets a test cover a body that yields from one block and resumes from another under `cleanup normal`, where the two exits disagree. 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 | 229 ++++++++++++++++--- 3 files changed, 227 insertions(+), 39 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..fe0a630f1d266 100644 --- a/clang/unittests/CIR/ControlFlowTest.cpp +++ b/clang/unittests/CIR/ControlFlowTest.cpp @@ -79,49 +79,67 @@ static void expectTerminatorSuccessors(Region ®ion, "region #" + std::to_string(region.getRegionNumber())); } +/// Return the branching terminators of `region`, one per block that ends in a +/// RegionBranchTerminatorOpInterface. Interior blocks that end in a plain +/// branch such as cir.brcond are not branch points and are skipped. +static SmallVector<RegionBranchTerminatorOpInterface> +getBranchTerminators(Region ®ion) { + SmallVector<RegionBranchTerminatorOpInterface> terminators; + for (Block &block : region) { + if (block.empty()) + continue; + if (auto term = dyn_cast<RegionBranchTerminatorOpInterface>(block.back())) + terminators.push_back(term); + } + return terminators; +} + /// 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 +/// - 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) { EXPECT_TRUE(succeeded(mlir::verify(op))) << "MLIR verifier failed for '" << op->getName().getStringRef().str() << "'"; - SmallVector<RegionSuccessor> entrySuccessors; - op.getSuccessorRegions(RegionBranchPoint::parent(), entrySuccessors); + // 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; - for (auto &succ : entrySuccessors) - allReachable.insert(succ.getSuccessor()); - for (Region ®ion : op->getRegions()) { - if (region.empty()) - continue; - RegionBranchTerminatorOpInterface term = getTerminator(region); + 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(); + 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)) << "Region #" << region.getRegionNumber() << " is non-empty but not reachable from any branch point"; + EXPECT_FALSE(getBranchTerminators(region).empty()) + << "Region #" << region.getRegionNumber() + << " has no RegionBranchTerminatorOpInterface terminator"; } EXPECT_TRUE(allReachable.contains(nullptr)) << "parent (exit) not reachable from any branch point"; @@ -513,7 +531,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 +544,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 +565,152 @@ 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, 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()}); + + // The cir.brcond block is not a branch point, so only the two exiting + // blocks show up here. + SmallVector<RegionBranchTerminatorOpInterface> bodyTerms = + getBranchTerminators(cleanupScopeOp.getBodyRegion()); + 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}); + + EXPECT_FALSE(asRegionBranch(cleanupScopeOp).hasLoop()); + + // The cleanup region does have a predecessor here, via the normal exit. + verifyControlFlowInterfaceConsistency(cleanupScopeOp); +} + TEST_F(CIRControlFlowTest, GlobalOpWithCtorAndDtor) { OwningOpRef<ModuleOp> module = parse(R"CIR( !s32i = !cir.int<s, 32> >From d7dc5d2993b06e711288caadeed5885885ca606f Mon Sep 17 00:00:00 2001 From: Henrich Lauko <[email protected]> Date: Fri, 4 Sep 2026 14:28:53 +0000 Subject: [PATCH 2/2] [CIR][NFC] Use CleanupKindAttr predicates in FlattenCFG FlattenCFG spelled the cleanup-kind tests three ways: raw enum comparisons against Normal and All in flattenCleanup, and `!= CleanupKind::Normal` twice in the match method. CleanupKindAttr already declares isNormal() and isEH() with exactly those meanings, and the loop path already reads the attribute rather than the enum, so use the predicates throughout. CleanupScopeOp::getSuccessorRegions depends on this same invariant. Stating it in one place means a later change to the kinds cannot leave the copies disagreeing. --- .../lib/CIR/Dialect/Transforms/FlattenCFG.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp b/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp index eda0b59cc2367..e045960463d34 100644 --- a/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp +++ b/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp @@ -1381,11 +1381,9 @@ class CIRCleanupScopeOpFlattening llvm::SmallVectorImpl<cir::ResumeOp> &resumeOpsToChain, mlir::PatternRewriter &rewriter) const { mlir::Location loc = cleanupOp.getLoc(); - cir::CleanupKind cleanupKind = cleanupOp.getCleanupKind(); - bool hasNormalCleanup = cleanupKind == cir::CleanupKind::Normal || - cleanupKind == cir::CleanupKind::All; - bool hasEHCleanup = cleanupKind == cir::CleanupKind::EH || - cleanupKind == cir::CleanupKind::All; + cir::CleanupKindAttr cleanupKind = cleanupOp.getCleanupKindAttr(); + bool hasNormalCleanup = cleanupKind.isNormal(); + bool hasEHCleanup = cleanupKind.isEH(); bool isMultiExit = exits.size() > 1; // Get references to region blocks before inlining. @@ -1648,7 +1646,7 @@ class CIRCleanupScopeOpFlattening if (hasNestedOpsToFlatten(cleanupOp.getBodyRegion())) return mlir::failure(); - cir::CleanupKind cleanupKind = cleanupOp.getCleanupKind(); + bool hasEHCleanup = cleanupOp.getCleanupKindAttr().isEH(); // Collect all exits from the body region. llvm::SmallVector<CleanupExit> exits; @@ -1658,11 +1656,11 @@ class CIRCleanupScopeOpFlattening assert(!exits.empty() && "cleanup scope body has no exit"); // Collect non-nothrow calls and throws that need to be converted to - // try_call/try_throw. This is only needed for EH and All cleanup kinds, - // but the vectors will simply be empty for Normal cleanup. + // try_call/try_throw. Both vectors stay empty unless the cleanup runs + // while unwinding. llvm::SmallVector<cir::CallOp> callsToRewrite; llvm::SmallVector<cir::ThrowOp> throwsToRewrite; - if (cleanupKind != cir::CleanupKind::Normal) { + if (hasEHCleanup) { collectThrowingCalls(cleanupOp.getBodyRegion(), callsToRewrite); collectThrows(cleanupOp.getBodyRegion(), throwsToRewrite); } @@ -1670,7 +1668,7 @@ class CIRCleanupScopeOpFlattening // Collect resume ops from already-flattened inner cleanup scopes that // need to chain through this cleanup's EH handler. llvm::SmallVector<cir::ResumeOp> resumeOpsToChain; - if (cleanupKind != cir::CleanupKind::Normal) + if (hasEHCleanup) collectResumeOps(cleanupOp.getBodyRegion(), resumeOpsToChain); return flattenCleanup(cleanupOp, exits, callsToRewrite, throwsToRewrite, _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
