[Mlir-commits] [mlir] [mlir] Fix false positive loops in RegionBranchOpInterface::hasLoop (PR #233784)
Henrich Lauko
llvmlistbot at llvm.org
Tue Oct 20 03:47:32 PDT 2026
https://github.com/xlauko created https://github.com/llvm/llvm-project/pull/233784
`RegionBranchOpInterface::hasLoop` reported a loop whenever its region graph traversal popped an already visited region, but the worklist holds one entry per successor of each terminator, so a region reached along two paths was popped twice even without a cycle (e.g. a region with two terminators that both branch to the same successor region, as in CIR's `cir.cleanup.scope` with a yield and a resume block). `hasLoop` now reports a loop only if some region reachable from the parent op can reach itself, the same check `isRepetitiveRegion` uses, and the now unused `visited` argument of the traversal stop condition is removed. New unit tests cover a region with two exits to the same successor (no loop) and a loop in a region reached only through the entry region.
Assisted-by: Claude Code (Claude Opus 5.5).
>From fe27f360dd69b2a9407c806aea4d61ef63f4aed5 Mon Sep 17 00:00:00 2001
From: Henrich Lauko <hlauko at nvidia.com>
Date: Tue, 20 Oct 2026 10:20:53 +0000
Subject: [PATCH] [mlir] Fix false positive loops in
RegionBranchOpInterface::hasLoop
hasLoop traversed the region graph from each entry region and reported
a loop as soon as it popped an already visited region. The worklist
gets one entry per successor of each terminator, so a region reached
along two paths was popped twice even without a cycle, e.g. when a
region has two terminators that both branch to the same successor
region. hasLoop now reports a loop only if some region reachable from
the parent op can reach itself, using the same check as
isRepetitiveRegion. The visited argument of the traversal stop
condition is no longer used and is removed.
Assisted-by: Claude Code (Claude Opus 5.5)
---
mlir/lib/Interfaces/ControlFlowInterfaces.cpp | 53 ++++--------
.../Interfaces/ControlFlowInterfacesTest.cpp | 86 ++++++++++++++++++-
2 files changed, 101 insertions(+), 38 deletions(-)
diff --git a/mlir/lib/Interfaces/ControlFlowInterfaces.cpp b/mlir/lib/Interfaces/ControlFlowInterfaces.cpp
index c2a2f7e2561096..060cb99b3a51c7 100644
--- a/mlir/lib/Interfaces/ControlFlowInterfaces.cpp
+++ b/mlir/lib/Interfaces/ControlFlowInterfaces.cpp
@@ -222,10 +222,8 @@ LogicalResult detail::verifyRegionBranchOpInterface(Operation *op) {
}
/// Stop condition for `traverseRegionGraph`. The traversal is interrupted if
-/// this function returns "true" for a successor region. The first parameter is
-/// the successor region. The second parameter indicates all already visited
-/// regions.
-using StopConditionFn = function_ref<bool(Region *, ArrayRef<bool> visited)>;
+/// this function returns "true" for a successor region.
+using StopConditionFn = function_ref<bool(Region *)>;
/// Traverse the region graph starting at `begin`. The traversal is interrupted
/// if `stopCondition` evaluates to "true" for a successor region. In that case,
@@ -280,7 +278,7 @@ static bool traverseRegionGraph(Region *begin,
LDBG() << "Processing region #" << nextRegion->getRegionNumber()
<< " from worklist (remaining: " << worklist.size() << ")";
- if (stopConditionFn(nextRegion, visited)) {
+ if (stopConditionFn(nextRegion)) {
LDBG() << "Stop condition met for region #"
<< nextRegion->getRegionNumber() << ", returning true";
return true;
@@ -309,11 +307,10 @@ static bool traverseRegionGraph(Region *begin,
static bool isRegionReachable(Region *begin, Region *r) {
assert(begin->getParentOp() == r->getParentOp() &&
"expected that both regions belong to the same op");
- return traverseRegionGraph(begin,
- [&](Region *nextRegion, ArrayRef<bool> visited) {
- // Interrupt traversal if `r` was reached.
- return nextRegion == r;
- });
+ return traverseRegionGraph(begin, [&](Region *nextRegion) {
+ // Interrupt traversal if `r` was reached.
+ return nextRegion == r;
+ });
}
/// Return `true` if `a` and `b` are in mutually exclusive regions.
@@ -405,35 +402,17 @@ bool RegionBranchOpInterface::hasLoop() {
LDBG() << "Checking if operation " << getOperation()->getName()
<< " has loops";
+ // There is a loop iff some region reachable from the parent op can reach
+ // itself. Reaching a region along several paths (e.g., from multiple
+ // terminators that branch to the same successor region) is not a loop.
SmallVector<RegionSuccessor> entryRegions;
getSuccessorRegions(RegionBranchPoint::parent(), entryRegions);
- LDBG() << "Found " << entryRegions.size() << " entry regions";
-
- for (RegionSuccessor successor : entryRegions) {
- if (successor.isRegion()) {
- LDBG() << "Checking entry region #"
- << successor.getSuccessor()->getRegionNumber() << " for loops";
-
- bool hasLoop =
- traverseRegionGraph(successor.getSuccessor(),
- [](Region *nextRegion, ArrayRef<bool> visited) {
- // Interrupt traversal if the region was already
- // visited.
- return visited[nextRegion->getRegionNumber()];
- });
-
- if (hasLoop) {
- LDBG() << "Found loop in entry region #"
- << successor.getSuccessor()->getRegionNumber();
- return true;
- }
- } else {
- LDBG() << "Skipping operation successor";
- }
- }
-
- LDBG() << "No loops found in operation";
- return false;
+ return llvm::any_of(entryRegions, [](RegionSuccessor successor) {
+ return successor.isRegion() &&
+ traverseRegionGraph(successor.getSuccessor(), [](Region *region) {
+ return isRegionReachable(region, region);
+ });
+ });
}
OperandRange
diff --git a/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp b/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
index 5b5da0534393b0..1b3cc636878051 100644
--- a/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
+++ b/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
@@ -132,12 +132,46 @@ struct SequentialRegionsOp
SequentialRegionsOp>::getSuccessorRegions;
};
+/// Region 0 is entered from the parent op and branches to region 1, which
+/// branches back to the parent op. With the `loop` attribute, region 1 also
+/// branches to itself.
+struct EntryRegionsOp
+ : public Op<EntryRegionsOp, RegionBranchOpInterface::Trait> {
+ using Op::Op;
+ static ArrayRef<StringRef> getAttributeNames() { return {}; }
+
+ static StringRef getOperationName() { return "cftest.entry_regions_op"; }
+
+ void getSuccessorRegions(RegionBranchPoint point,
+ SmallVectorImpl<RegionSuccessor> ®ions) {
+ if (point.isParent()) {
+ regions.push_back(RegionSuccessor(&(*this)->getRegion(0)));
+ return;
+ }
+ if (point.getTerminatorPredecessorOrNull()->getParentRegion() ==
+ &(*this)->getRegion(0)) {
+ regions.push_back(RegionSuccessor(&(*this)->getRegion(1)));
+ return;
+ }
+ if ((*this)->hasAttr("loop"))
+ regions.push_back(RegionSuccessor(&(*this)->getRegion(1)));
+ regions.push_back(RegionSuccessor(getOperation()));
+ }
+
+ ValueRange getSuccessorInputs(RegionSuccessor successor) {
+ return successor.isOperation() ? ValueRange(getOperation()->getResults())
+ : ValueRange();
+ }
+
+ using RegionBranchOpInterface::Trait<EntryRegionsOp>::getSuccessorRegions;
+};
+
/// A dialect putting all the above together.
struct CFTestDialect : Dialect {
explicit CFTestDialect(MLIRContext *ctx)
: Dialect(getDialectNamespace(), ctx, TypeID::get<CFTestDialect>()) {
addOperations<DummyOp, MutuallyExclusiveRegionsOp, LoopRegionsOp,
- DoubleLoopRegionsOp, SequentialRegionsOp>();
+ DoubleLoopRegionsOp, SequentialRegionsOp, EntryRegionsOp>();
}
static StringRef getDialectNamespace() { return "cftest"; }
};
@@ -284,3 +318,53 @@ TEST(RegionBranchOpInterface, NotRecursiveRegions) {
EXPECT_EQ(getEnclosingRepetitiveRegion(op1), nullptr);
EXPECT_EQ(getEnclosingRepetitiveRegion(op2), nullptr);
}
+
+TEST(RegionBranchOpInterface, MultipleExitsToSameRegionIsNotALoop) {
+ // Both terminators of region 0 branch to region 1. Region 1 is reached
+ // twice, but the region graph has no cycle.
+ const char *ir = R"MLIR(
+"cftest.entry_regions_op"() (
+ {
+ "cftest.dummy_op"() : () -> ()
+ ^bb1:
+ "cftest.dummy_op"() : () -> ()
+ },
+ {"cftest.dummy_op"() : () -> ()}
+ ) : () -> ()
+ )MLIR";
+
+ DialectRegistry registry;
+ registry.insert<CFTestDialect>();
+ MLIRContext ctx(registry);
+
+ OwningOpRef<ModuleOp> module = parseSourceString<ModuleOp>(ir, &ctx);
+ ASSERT_TRUE(module);
+ auto regionOp = cast<RegionBranchOpInterface>(
+ &module->getBody()->getOperations().front());
+
+ EXPECT_FALSE(regionOp.isRepetitiveRegion(0));
+ EXPECT_FALSE(regionOp.isRepetitiveRegion(1));
+ EXPECT_FALSE(regionOp.hasLoop());
+}
+
+TEST(RegionBranchOpInterface, LoopAfterEntryRegion) {
+ const char *ir = R"MLIR(
+"cftest.entry_regions_op"() (
+ {"cftest.dummy_op"() : () -> ()},
+ {"cftest.dummy_op"() : () -> ()}
+ ) {loop} : () -> ()
+ )MLIR";
+
+ DialectRegistry registry;
+ registry.insert<CFTestDialect>();
+ MLIRContext ctx(registry);
+
+ OwningOpRef<ModuleOp> module = parseSourceString<ModuleOp>(ir, &ctx);
+ ASSERT_TRUE(module);
+ auto regionOp = cast<RegionBranchOpInterface>(
+ &module->getBody()->getOperations().front());
+
+ EXPECT_FALSE(regionOp.isRepetitiveRegion(0));
+ EXPECT_TRUE(regionOp.isRepetitiveRegion(1));
+ EXPECT_TRUE(regionOp.hasLoop());
+}
More information about the Mlir-commits
mailing list