[Mlir-commits] [clang] [mlir] Reland "[mlir] Fix false positive loops in RegionBranchOpInterface::hasLoop" (PR #234591)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Fri Oct 23 04:57:11 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir
Author: Henrich Lauko (xlauko)
<details>
<summary>Changes</summary>
Relands #<!-- -->233784, reverted in #<!-- -->234573 because it broke two CIR unit
tests.
Those tests expected hasLoop() to be true for cir.await and
cir.coroutine, although neither has a cycle: resume is reachable from
ready both directly and through suspend, and the coroutine reaches exit
and destroy along several paths. That was the false positive this change
fixes. The relanded change updates both expectations to false.
Original description:
`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)
---
Full diff: https://github.com/llvm/llvm-project/pull/234591.diff
3 Files Affected:
- (modified) clang/unittests/CIR/ControlFlowTest.cpp (+2-5)
- (modified) mlir/lib/Interfaces/ControlFlowInterfaces.cpp (+92-68)
- (modified) mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp (+129-1)
``````````diff
diff --git a/clang/unittests/CIR/ControlFlowTest.cpp b/clang/unittests/CIR/ControlFlowTest.cpp
index e3a6d46ffdbbba..1cad991d860b72 100644
--- a/clang/unittests/CIR/ControlFlowTest.cpp
+++ b/clang/unittests/CIR/ControlFlowTest.cpp
@@ -824,7 +824,7 @@ TEST_F(CIRControlFlowTest, CoroutineOp) {
EXPECT_FALSE(coroBranch.isRepetitiveRegion(2));
EXPECT_FALSE(coroBranch.isRepetitiveRegion(3));
EXPECT_FALSE(coroBranch.isRepetitiveRegion(4));
- EXPECT_TRUE(coroBranch.hasLoop());
+ EXPECT_FALSE(coroBranch.hasLoop());
// TODO: cir.return doesn't implement RegionBranchTerminatorOpInterface
// verifyControlFlowInterfaceConsistency(coroOp);
@@ -872,14 +872,11 @@ TEST_F(CIRControlFlowTest, AwaitOp) {
{&awaitOp.getResume(), nullptr});
expectTerminatorSuccessors(awaitOp.getResume(), {nullptr});
- // MLIR's RegionBranchOpInterface::hasLoop() reports any region reached
- // twice as a loop, and resume is reachable both directly from ready and
- // through suspend.
RegionBranchOpInterface awaitBranch = asRegionBranch(awaitOp);
EXPECT_FALSE(awaitBranch.isRepetitiveRegion(0));
EXPECT_FALSE(awaitBranch.isRepetitiveRegion(1));
EXPECT_FALSE(awaitBranch.isRepetitiveRegion(2));
- EXPECT_TRUE(awaitBranch.hasLoop());
+ EXPECT_FALSE(awaitBranch.hasLoop());
verifyControlFlowInterfaceConsistency(awaitOp);
}
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());
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/234591
More information about the Mlir-commits
mailing list