llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clangir
Author: Henrich Lauko (xlauko)
<details>
<summary>Changes</summary>
`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).
---
Full diff: https://github.com/llvm/llvm-project/pull/221222.diff
3 Files Affected:
- (modified) clang/include/clang/CIR/Dialect/IR/CIROps.td (+4-5)
- (modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+22-5)
- (modified) clang/unittests/CIR/ControlFlowTest.cpp (+162-37)
``````````diff
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td
b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index aa32ada090f917..debe973dd1f9a4 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 0fb9f585d2228c..9679a577a48314 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -2037,14 +2037,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 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);
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/221222
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
