[Mlir-commits] [mlir] ccd6bf9 - [mlir] Fix false positive loops in RegionBranchOpInterface::hasLoop (#233784)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Thu Oct 22 03:07:33 PDT 2026
Author: Henrich Lauko
Date: 2026-10-22T12:07:28+02:00
New Revision: ccd6bf94945635d83580995d73f5970fa2d20490
URL: https://github.com/llvm/llvm-project/commit/ccd6bf94945635d83580995d73f5970fa2d20490
DIFF: https://github.com/llvm/llvm-project/commit/ccd6bf94945635d83580995d73f5970fa2d20490.diff
LOG: [mlir] Fix false positive loops in RegionBranchOpInterface::hasLoop (#233784)
`RegionBranchOpInterface::hasLoop` reported a loop whenever its
traversal popped a region it had already visited. The worklist gets one
entry per successor of each terminator, so a region reachable along two
paths was popped twice even without a cycle, for example when two
terminators of a region branch to the same successor region.
`hasLoop` now runs one iterative depth-first search from the entry
regions and reports a loop only when it reaches a region that is still
on the DFS stack. Each region is queried at most once, through
`RegionBranchOpInterface::getSuccessorRegions(Region &)` instead of
asking each terminator with null operand attributes. The stop condition
of the remaining traversal loses its `visited` argument.
Assisted-by: Claude Code (Claude Opus 5.5).
Added:
Modified:
mlir/lib/Interfaces/ControlFlowInterfaces.cpp
mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
Removed:
################################################################################
diff --git a/mlir/lib/Interfaces/ControlFlowInterfaces.cpp b/mlir/lib/Interfaces/ControlFlowInterfaces.cpp
index c2a2f7e2561096..3f4f5d7691df4d 100644
--- a/mlir/lib/Interfaces/ControlFlowInterfaces.cpp
+++ b/mlir/lib/Interfaces/ControlFlowInterfaces.cpp
@@ -15,6 +15,7 @@
#include "mlir/IR/PatternMatch.h"
#include "mlir/Interfaces/ControlFlowInterfaces.h"
#include "llvm/ADT/EquivalenceClasses.h"
+#include "llvm/ADT/PointerIntPair.h"
#include "llvm/Support/DebugLog.h"
using namespace mlir;
@@ -221,11 +222,34 @@ LogicalResult detail::verifyRegionBranchOpInterface(Operation *op) {
return success();
}
+/// Return the regions in `successors`. Branches back to the parent op are left
+/// out.
+static SmallVector<Region *>
+filterRegionSuccessors(ArrayRef<RegionSuccessor> successors) {
+ SmallVector<Region *> regions;
+ for (RegionSuccessor successor : successors)
+ if (successor.isRegion())
+ regions.push_back(successor.getSuccessor());
+ return regions;
+}
+
+/// Return the regions that the terminators of `region` may branch to.
+///
+/// This asks the parent op, not each terminator. A terminator's own
+/// `getSuccessorRegions` can only narrow the parent's answer using constant
+/// operands, and we know no operand values here.
+static SmallVector<Region *> getSuccessorRegionsOf(Region *region) {
+ SmallVector<RegionSuccessor> successors;
+ cast<RegionBranchOpInterface>(region->getParentOp())
+ .getSuccessorRegions(*region, successors);
+ LDBG() << "Region #" << region->getRegionNumber() << " has "
+ << successors.size() << " successors";
+ return filterRegionSuccessors(successors);
+}
+
/// 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,
@@ -242,36 +266,7 @@ static bool traverseRegionGraph(Region *begin,
LDBG() << "Initialized visited array with " << op->getNumRegions()
<< " regions";
- // Retrieve all successors of the region and enqueue them in the worklist.
- SmallVector<Region *> worklist;
- auto enqueueAllSuccessors = [&](Region *region) {
- LDBG() << "Enqueuing successors for region #" << region->getRegionNumber();
- SmallVector<Attribute> operandAttributes(op->getNumOperands());
- for (Block &block : *region) {
- if (block.empty())
- continue;
- auto terminator =
- dyn_cast<RegionBranchTerminatorOpInterface>(block.back());
- if (!terminator)
- continue;
- SmallVector<RegionSuccessor> successors;
- operandAttributes.resize(terminator->getNumOperands());
- terminator.getSuccessorRegions(operandAttributes, successors);
- LDBG() << "Found " << successors.size()
- << " successors from terminator in block";
- for (RegionSuccessor successor : successors) {
- if (successor.isRegion()) {
- worklist.push_back(successor.getSuccessor());
- LDBG() << "Added region #"
- << successor.getSuccessor()->getRegionNumber()
- << " to worklist";
- } else {
- LDBG() << "Skipping operation successor";
- }
- }
- }
- };
- enqueueAllSuccessors(begin);
+ SmallVector<Region *> worklist = getSuccessorRegionsOf(begin);
LDBG() << "Initial worklist size: " << worklist.size();
// Process all regions in the worklist via DFS.
@@ -280,7 +275,9 @@ static bool traverseRegionGraph(Region *begin,
LDBG() << "Processing region #" << nextRegion->getRegionNumber()
<< " from worklist (remaining: " << worklist.size() << ")";
- if (stopConditionFn(nextRegion, visited)) {
+ // Check the stop condition before the visited set. `begin` starts out
+ // visited, and reaching it again must still stop the traversal.
+ if (stopConditionFn(nextRegion)) {
LDBG() << "Stop condition met for region #"
<< nextRegion->getRegionNumber() << ", returning true";
return true;
@@ -297,7 +294,7 @@ static bool traverseRegionGraph(Region *begin,
visited[nextRegion->getRegionNumber()] = true;
LDBG() << "Marking region #" << nextRegion->getRegionNumber()
<< " as visited";
- enqueueAllSuccessors(nextRegion);
+ llvm::append_range(worklist, getSuccessorRegionsOf(nextRegion));
}
LDBG() << "Traversal completed, returning false";
@@ -309,11 +306,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.
@@ -401,39 +397,67 @@ bool RegionBranchOpInterface::isRepetitiveRegion(unsigned index) {
return isRepetitive;
}
+/// Return "true" if a cycle in the region graph is reachable from one of
+/// `entryRegions`, which must belong to the same op.
+///
+/// This is an iterative depth-first search. A region is on the DFS path from
+/// the time the search enters it until every region reachable from it is
+/// finished. Reaching a region on the path closes a cycle. Reaching a finished
+/// region does not. That happens, for example, when two terminators branch to
+/// the same successor region.
+static bool canReachCycle(ArrayRef<Region *> entryRegions) {
+ // With no entry regions, nothing is reachable. `states` below also needs an
+ // entry region to get the op from.
+ if (entryRegions.empty())
+ return false;
+ enum class State : uint8_t { Unvisited, OnPath, Finished };
+ SmallVector<State> states(
+ entryRegions.front()->getParentOp()->getNumRegions(), State::Unvisited);
+
+ // Each stack item enters its region, or leaves it if the flag is set.
+ // Entering a region pushes its leave item first and the enter items of its
+ // successors on top. The leave item pops only after all of those finish, so
+ // the region stays on the path until then.
+ using StackItem = llvm::PointerIntPair<Region *, 1, bool>;
+ SmallVector<StackItem> stack;
+ for (Region *region : entryRegions)
+ stack.push_back(StackItem(region, /*isLeave=*/false));
+ while (!stack.empty()) {
+ StackItem item = stack.pop_back_val();
+ Region *region = item.getPointer();
+ bool isLeave = item.getInt();
+ State &state = states[region->getRegionNumber()];
+ if (isLeave) {
+ state = State::Finished;
+ continue;
+ }
+ if (state == State::OnPath) {
+ LDBG() << "Found loop through region #" << region->getRegionNumber();
+ return true;
+ }
+ if (state == State::Finished)
+ continue;
+
+ state = State::OnPath;
+ stack.push_back(StackItem(region, /*isLeave=*/true));
+ for (Region *successor : getSuccessorRegionsOf(region))
+ stack.push_back(StackItem(successor, /*isLeave=*/false));
+ }
+ return false;
+}
+
bool RegionBranchOpInterface::hasLoop() {
LDBG() << "Checking if operation " << getOperation()->getName()
<< " has loops";
- SmallVector<RegionSuccessor> entryRegions;
- getSuccessorRegions(RegionBranchPoint::parent(), entryRegions);
+ SmallVector<RegionSuccessor> entrySuccessors;
+ getSuccessorRegions(RegionBranchPoint::parent(), entrySuccessors);
+ SmallVector<Region *> entryRegions = filterRegionSuccessors(entrySuccessors);
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;
+ bool hasLoop = canReachCycle(entryRegions);
+ LDBG() << "Operation " << (hasLoop ? "has" : "has no") << " loops";
+ return hasLoop;
}
OperandRange
diff --git a/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp b/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
index 5b5da0534393b0..df581d481644f4 100644
--- a/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
+++ b/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "mlir/Interfaces/ControlFlowInterfaces.h"
+#include "mlir/IR/Builders.h"
#include "mlir/IR/BuiltinOps.h"
#include "mlir/IR/Dialect.h"
#include "mlir/IR/DialectImplementation.h"
@@ -132,12 +133,49 @@ struct SequentialRegionsOp
SequentialRegionsOp>::getSuccessorRegions;
};
+/// Two attributes define the region graph of this op. `entries` lists the
+/// regions that the op enters. `successors[i]` lists the regions that each
+/// terminator of region i branches to. Every terminator can also branch back to
+/// the op.
+struct RegionGraphOp
+ : public Op<RegionGraphOp, RegionBranchOpInterface::Trait> {
+ using Op::Op;
+ static ArrayRef<StringRef> getAttributeNames() { return {}; }
+
+ static StringRef getOperationName() { return "cftest.region_graph_op"; }
+
+ void getSuccessorRegions(RegionBranchPoint point,
+ SmallVectorImpl<RegionSuccessor> ®ions) {
+ Operation *op = getOperation();
+ DenseI64ArrayAttr targets;
+ if (point.isParent()) {
+ targets = cast<DenseI64ArrayAttr>(op->getAttr("entries"));
+ } else {
+ // Branching back to the op adds no edge between regions. It only lets
+ // every region exit.
+ regions.push_back(RegionSuccessor(op));
+ Region *from = point.getTerminatorPredecessorOrNull()->getParentRegion();
+ targets = cast<DenseI64ArrayAttr>(
+ cast<ArrayAttr>(op->getAttr("successors"))[from->getRegionNumber()]);
+ }
+ for (int64_t target : targets.asArrayRef())
+ regions.push_back(RegionSuccessor(&op->getRegion(target)));
+ }
+
+ ValueRange getSuccessorInputs(RegionSuccessor successor) {
+ return successor.isOperation() ? ValueRange(getOperation()->getResults())
+ : ValueRange();
+ }
+
+ using RegionBranchOpInterface::Trait<RegionGraphOp>::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, RegionGraphOp>();
}
static StringRef getDialectNamespace() { return "cftest"; }
};
@@ -284,3 +322,93 @@ TEST(RegionBranchOpInterface, NotRecursiveRegions) {
EXPECT_EQ(getEnclosingRepetitiveRegion(op1), nullptr);
EXPECT_EQ(getEnclosingRepetitiveRegion(op2), nullptr);
}
+
+/// Builds `cftest.region_graph_op` ops and keeps them alive for the test.
+class RegionGraphTest : public ::testing::Test {
+protected:
+ RegionGraphTest() : builder(&ctx) {
+ ctx.loadDialect<CFTestDialect>();
+ module = ModuleOp::create(builder.getUnknownLoc());
+ builder.setInsertionPointToEnd(module->getBody());
+ }
+
+ /// Create a `cftest.region_graph_op` with one region per entry of
+ /// `successors`. Each region has one block, whose terminator branches to the
+ /// regions listed for it.
+ RegionBranchOpInterface
+ createRegionGraphOp(ArrayRef<int64_t> entries,
+ ArrayRef<SmallVector<int64_t>> successors) {
+ OperationState state(builder.getUnknownLoc(),
+ RegionGraphOp::getOperationName());
+ SmallVector<Attribute> successorAttrs = llvm::map_to_vector(
+ successors, [&](ArrayRef<int64_t> regions) -> Attribute {
+ return builder.getDenseI64ArrayAttr(regions);
+ });
+ state.addAttribute("entries", builder.getDenseI64ArrayAttr(entries));
+ state.addAttribute("successors", builder.getArrayAttr(successorAttrs));
+ for (size_t i = 0, e = successors.size(); i < e; ++i)
+ state.addRegion();
+ Operation *op = builder.create(state);
+ for (Region ®ion : op->getRegions())
+ addTerminatedBlock(region);
+ return cast<RegionBranchOpInterface>(op);
+ }
+
+ /// Append a block to `region` that holds only a region branch terminator.
+ void addTerminatedBlock(Region ®ion) {
+ OpBuilder::InsertionGuard guard(builder);
+ builder.createBlock(®ion);
+ builder.create(
+ OperationState(builder.getUnknownLoc(), DummyOp::getOperationName()));
+ }
+
+ MLIRContext ctx;
+ OpBuilder builder;
+ OwningOpRef<ModuleOp> module;
+};
+
+TEST_F(RegionGraphTest, MultipleBranchesToSameRegionIsNotALoop) {
+ RegionBranchOpInterface op =
+ createRegionGraphOp(/*entries=*/{0}, /*successors=*/{{1}, {}});
+ // Give region 0 a second terminator. Both terminators branch to region 1.
+ addTerminatedBlock(op->getRegion(0));
+ EXPECT_FALSE(op.isRepetitiveRegion(0));
+ EXPECT_FALSE(op.isRepetitiveRegion(1));
+ EXPECT_FALSE(op.hasLoop());
+}
+
+TEST_F(RegionGraphTest, DiamondIsNotALoop) {
+ RegionBranchOpInterface op = createRegionGraphOp(
+ /*entries=*/{0}, /*successors=*/{{1, 2}, {3}, {3}, {}});
+ EXPECT_FALSE(op.hasLoop());
+}
+
+TEST_F(RegionGraphTest, LoopAfterEntryRegion) {
+ RegionBranchOpInterface op =
+ createRegionGraphOp(/*entries=*/{0}, /*successors=*/{{1}, {1}});
+ EXPECT_FALSE(op.isRepetitiveRegion(0));
+ EXPECT_TRUE(op.isRepetitiveRegion(1));
+ EXPECT_TRUE(op.hasLoop());
+}
+
+TEST_F(RegionGraphTest, LoopThroughEntryRegion) {
+ RegionBranchOpInterface op =
+ createRegionGraphOp(/*entries=*/{0}, /*successors=*/{{1}, {0}});
+ EXPECT_TRUE(op.hasLoop());
+}
+
+TEST_F(RegionGraphTest, UnreachableLoopIsIgnored) {
+ RegionBranchOpInterface op =
+ createRegionGraphOp(/*entries=*/{0}, /*successors=*/{{1}, {}, {2}});
+ EXPECT_TRUE(op.isRepetitiveRegion(2));
+ EXPECT_FALSE(op.hasLoop());
+}
+
+TEST_F(RegionGraphTest, OverlappingEntriesAreNotALoop) {
+ // Region 1 is an entry region and is also reachable from entry region 0.
+ // The search must not depend on the order of the entry regions.
+ EXPECT_FALSE(createRegionGraphOp(/*entries=*/{0, 1}, /*successors=*/{{1}, {}})
+ .hasLoop());
+ EXPECT_FALSE(createRegionGraphOp(/*entries=*/{1, 0}, /*successors=*/{{1}, {}})
+ .hasLoop());
+}
More information about the Mlir-commits
mailing list