[Mlir-commits] [mlir] [mlir] Fix false positive loops in RegionBranchOpInterface::hasLoop (PR #233784)

Henrich Lauko llvmlistbot at llvm.org
Wed Oct 21 01:03:30 PDT 2026


https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/233784

>From 16010ab9b5786c84c69adc3e18e793eff2f47241 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 1/2] [mlir] Fix false positive loops in
 RegionBranchOpInterface::hasLoop

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 reports a loop only if a region reachable from the parent op
can reach itself, which is the check isRepetitiveRegion already does.
Drop the now unused visited argument of the traversal stop condition.

Assisted-by: Claude Code (Claude Opus 5.5)
---
 mlir/lib/Interfaces/ControlFlowInterfaces.cpp | 53 ++++--------
 .../Interfaces/ControlFlowInterfacesTest.cpp  | 84 ++++++++++++++++++-
 2 files changed, 99 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..d11764d87eb1a7 100644
--- a/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
+++ b/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
@@ -132,12 +132,44 @@ 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> &regions) {
+    Operation *op = getOperation();
+    if (point.isParent()) {
+      regions.push_back(RegionSuccessor(&op->getRegion(0)));
+      return;
+    }
+    Region *from = point.getTerminatorPredecessorOrNull()->getParentRegion();
+    if (from == &op->getRegion(0) || op->hasAttr("loop"))
+      regions.push_back(RegionSuccessor(&op->getRegion(1)));
+    if (from == &op->getRegion(1))
+      regions.push_back(RegionSuccessor(op));
+  }
+
+  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 +316,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());
+}

>From 0ac0c3f83ebf5fab798277e1daa5b9d22e1645ff Mon Sep 17 00:00:00 2001
From: Henrich Lauko <hlauko at nvidia.com>
Date: Wed, 21 Oct 2026 07:36:16 +0000
Subject: [PATCH 2/2] [mlir] Detect hasLoop cycles with scc_iterator

Running isRegionReachable on every region the traversal pops made
hasLoop quadratic in the number of region successor edges. Build the
region graph reachable from the op once, querying each terminator a
single time, and look for a cyclic SCC with llvm::scc_iterator. The
terminator walk moves into getRegionSuccessors, shared with
traverseRegionGraph.

Restore the dropped debug output, rename the multi-branch test, and
replace its mock op with one whose region graph comes from attributes.
New tests cover a diamond, a loop through the entry region, a loop in
an unreachable region and overlapping entry regions.

Assisted-by: Claude Code (Claude Opus 5.5)
---
 mlir/lib/Interfaces/ControlFlowInterfaces.cpp | 161 +++++++++++++-----
 .../Interfaces/ControlFlowInterfacesTest.cpp  | 150 ++++++++++------
 2 files changed, 219 insertions(+), 92 deletions(-)

diff --git a/mlir/lib/Interfaces/ControlFlowInterfaces.cpp b/mlir/lib/Interfaces/ControlFlowInterfaces.cpp
index 060cb99b3a51c7..abfb1488bea0e9 100644
--- a/mlir/lib/Interfaces/ControlFlowInterfaces.cpp
+++ b/mlir/lib/Interfaces/ControlFlowInterfaces.cpp
@@ -15,6 +15,8 @@
 #include "mlir/IR/PatternMatch.h"
 #include "mlir/Interfaces/ControlFlowInterfaces.h"
 #include "llvm/ADT/EquivalenceClasses.h"
+#include "llvm/ADT/GraphTraits.h"
+#include "llvm/ADT/SCCIterator.h"
 #include "llvm/Support/DebugLog.h"
 
 using namespace mlir;
@@ -221,6 +223,35 @@ LogicalResult detail::verifyRegionBranchOpInterface(Operation *op) {
   return success();
 }
 
+/// Append the region successors of all terminators in `region` to
+/// `successors`. Operation successors are skipped.
+static void getRegionSuccessors(Region *region,
+                                SmallVectorImpl<Region *> &successors) {
+  LDBG() << "Collecting successors of region #" << region->getRegionNumber();
+  SmallVector<Attribute> operandAttributes;
+  for (Block &block : *region) {
+    if (block.empty())
+      continue;
+    auto terminator = dyn_cast<RegionBranchTerminatorOpInterface>(block.back());
+    if (!terminator)
+      continue;
+    SmallVector<RegionSuccessor> regionSuccessors;
+    operandAttributes.resize(terminator->getNumOperands());
+    terminator.getSuccessorRegions(operandAttributes, regionSuccessors);
+    LDBG() << "Found " << regionSuccessors.size()
+           << " successors from terminator in block";
+    for (RegionSuccessor successor : regionSuccessors) {
+      if (successor.isRegion()) {
+        successors.push_back(successor.getSuccessor());
+        LDBG() << "Added region #"
+               << successor.getSuccessor()->getRegionNumber();
+      } else {
+        LDBG() << "Skipping operation successor";
+      }
+    }
+  }
+}
+
 /// Stop condition for `traverseRegionGraph`. The traversal is interrupted if
 /// this function returns "true" for a successor region.
 using StopConditionFn = function_ref<bool(Region *)>;
@@ -240,36 +271,8 @@ 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);
+  getRegionSuccessors(begin, worklist);
   LDBG() << "Initial worklist size: " << worklist.size();
 
   // Process all regions in the worklist via DFS.
@@ -295,7 +298,7 @@ static bool traverseRegionGraph(Region *begin,
     visited[nextRegion->getRegionNumber()] = true;
     LDBG() << "Marking region #" << nextRegion->getRegionNumber()
            << " as visited";
-    enqueueAllSuccessors(nextRegion);
+    getRegionSuccessors(nextRegion, worklist);
   }
 
   LDBG() << "Traversal completed, returning false";
@@ -398,21 +401,99 @@ bool RegionBranchOpInterface::isRepetitiveRegion(unsigned index) {
   return isRepetitive;
 }
 
+namespace {
+/// The region graph of a RegionBranchOpInterface op, restricted to the regions
+/// that are reachable from the op. The entry node stands for the op itself and
+/// has an edge to each entry region. Branches back to the op are not edges, so
+/// the entry node is never part of a cycle.
+class RegionBranchGraph {
+public:
+  struct Node {
+    /// The region of this node, or nullptr for the entry node and for regions
+    /// that are not reachable.
+    Region *region = nullptr;
+    SmallVector<Node *> successors;
+  };
+
+  explicit RegionBranchGraph(RegionBranchOpInterface op);
+
+  Node *getEntry() { return &nodes.back(); }
+
+private:
+  /// One node per region, followed by the entry node. The size is fixed on
+  /// construction, so pointers to nodes stay valid.
+  SmallVector<Node> nodes;
+};
+} // namespace
+
+RegionBranchGraph::RegionBranchGraph(RegionBranchOpInterface op)
+    : nodes(op->getNumRegions() + 1) {
+  SmallVector<Node *> worklist;
+  // Add an edge from `from` to each of `regions`. Enqueue each region when it
+  // is first reached.
+  auto addEdges = [&](Node *from, ArrayRef<Region *> regions) {
+    for (Region *region : regions) {
+      Node *to = &nodes[region->getRegionNumber()];
+      if (!to->region) {
+        to->region = region;
+        worklist.push_back(to);
+      }
+      from->successors.push_back(to);
+    }
+  };
+
+  SmallVector<RegionSuccessor> entrySuccessors;
+  op.getSuccessorRegions(RegionBranchPoint::parent(), entrySuccessors);
+  SmallVector<Region *> entryRegions;
+  for (RegionSuccessor successor : entrySuccessors)
+    if (successor.isRegion())
+      entryRegions.push_back(successor.getSuccessor());
+  addEdges(getEntry(), entryRegions);
+
+  while (!worklist.empty()) {
+    Node *node = worklist.pop_back_val();
+    SmallVector<Region *> successors;
+    getRegionSuccessors(node->region, successors);
+    addEdges(node, successors);
+  }
+}
+
+namespace llvm {
+template <>
+struct GraphTraits<RegionBranchGraph::Node *> {
+  using NodeRef = RegionBranchGraph::Node *;
+  using ChildIteratorType = SmallVectorImpl<NodeRef>::iterator;
+
+  static NodeRef getEntryNode(NodeRef node) { return node; }
+  static ChildIteratorType child_begin(NodeRef node) {
+    return node->successors.begin();
+  }
+  static ChildIteratorType child_end(NodeRef node) {
+    return node->successors.end();
+  }
+};
+} // namespace llvm
+
 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
+  RegionBranchGraph graph(*this);
+  LDBG() << "Found " << graph.getEntry()->successors.size() << " entry regions";
+
+  // There is a loop iff a strongly connected component of the region graph has
+  // a cycle. A region that is reached 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);
-  return llvm::any_of(entryRegions, [](RegionSuccessor successor) {
-    return successor.isRegion() &&
-           traverseRegionGraph(successor.getSuccessor(), [](Region *region) {
-             return isRegionReachable(region, region);
-           });
-  });
+  for (auto scc = llvm::scc_begin(graph.getEntry()); !scc.isAtEnd(); ++scc) {
+    if (scc.hasCycle()) {
+      LDBG() << "Found loop through region #"
+             << (*scc).front()->region->getRegionNumber();
+      return true;
+    }
+  }
+
+  LDBG() << "No loops found in operation";
+  return false;
 }
 
 OperandRange
diff --git a/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp b/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
index d11764d87eb1a7..d68e686be82b75 100644
--- a/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
+++ b/mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp
@@ -132,28 +132,29 @@ 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> {
+/// The region graph of this op is given by attributes: `entries` lists the
+/// regions that are entered from the op, and `successors[i]` lists the regions
+/// that each terminator of region i branches to. Each terminator also branches
+/// back to the op.
+struct RegionGraphOp
+    : public Op<RegionGraphOp, RegionBranchOpInterface::Trait> {
   using Op::Op;
   static ArrayRef<StringRef> getAttributeNames() { return {}; }
 
-  static StringRef getOperationName() { return "cftest.entry_regions_op"; }
+  static StringRef getOperationName() { return "cftest.region_graph_op"; }
 
   void getSuccessorRegions(RegionBranchPoint point,
                            SmallVectorImpl<RegionSuccessor> &regions) {
     Operation *op = getOperation();
-    if (point.isParent()) {
-      regions.push_back(RegionSuccessor(&op->getRegion(0)));
-      return;
-    }
-    Region *from = point.getTerminatorPredecessorOrNull()->getParentRegion();
-    if (from == &op->getRegion(0) || op->hasAttr("loop"))
-      regions.push_back(RegionSuccessor(&op->getRegion(1)));
-    if (from == &op->getRegion(1))
+    Attribute targets = op->getAttr("entries");
+    if (!point.isParent()) {
+      Region *from = point.getTerminatorPredecessorOrNull()->getParentRegion();
+      targets = cast<ArrayAttr>(op->getAttr("successors"))
+                    .getValue()[from->getRegionNumber()];
       regions.push_back(RegionSuccessor(op));
+    }
+    for (int64_t target : cast<DenseI64ArrayAttr>(targets).asArrayRef())
+      regions.push_back(RegionSuccessor(&op->getRegion(target)));
   }
 
   ValueRange getSuccessorInputs(RegionSuccessor successor) {
@@ -161,7 +162,7 @@ struct EntryRegionsOp
                                    : ValueRange();
   }
 
-  using RegionBranchOpInterface::Trait<EntryRegionsOp>::getSuccessorRegions;
+  using RegionBranchOpInterface::Trait<RegionGraphOp>::getSuccessorRegions;
 };
 
 /// A dialect putting all the above together.
@@ -169,7 +170,7 @@ struct CFTestDialect : Dialect {
   explicit CFTestDialect(MLIRContext *ctx)
       : Dialect(getDialectNamespace(), ctx, TypeID::get<CFTestDialect>()) {
     addOperations<DummyOp, MutuallyExclusiveRegionsOp, LoopRegionsOp,
-                  DoubleLoopRegionsOp, SequentialRegionsOp, EntryRegionsOp>();
+                  DoubleLoopRegionsOp, SequentialRegionsOp, RegionGraphOp>();
   }
   static StringRef getDialectNamespace() { return "cftest"; }
 };
@@ -317,52 +318,97 @@ TEST(RegionBranchOpInterface, NotRecursiveRegions) {
   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"() (
+/// Parses `cftest.region_graph_op` ops and keeps them alive for the test.
+class RegionGraphTest : public ::testing::Test {
+protected:
+  RegionGraphTest() { ctx.loadDialect<CFTestDialect>(); }
+
+  /// Parse `ir`, which holds a single op, and return that op.
+  RegionBranchOpInterface parse(StringRef ir) {
+    module = parseSourceString<ModuleOp>(ir, &ctx);
+    if (!module)
+      return nullptr;
+    return cast<RegionBranchOpInterface>(&module->getBody()->front());
+  }
+
+  /// Parse a `cftest.region_graph_op` with one single-block region per entry
+  /// of `successors`. `entries` and each entry of `successors` are
+  /// comma-separated region numbers.
+  RegionBranchOpInterface parse(StringRef entries,
+                                ArrayRef<StringRef> successors) {
+    auto toArray = [](StringRef values) -> std::string {
+      return values.empty() ? "array<i64>"
+                            : ("array<i64: " + values + ">").str();
+    };
+    std::string ir = "\"cftest.region_graph_op\"() (";
+    for (size_t i = 0, e = successors.size(); i < e; ++i)
+      ir += std::string(i ? ", " : "") + "{\"cftest.dummy_op\"() : () -> ()}";
+    ir += ") {entries = " + toArray(entries) + ", successors = [";
+    for (size_t i = 0, e = successors.size(); i < e; ++i)
+      ir += std::string(i ? ", " : "") + toArray(successors[i]);
+    ir += "]} : () -> ()";
+    return parse(ir);
+  }
+
+  MLIRContext ctx;
+  OwningOpRef<ModuleOp> module;
+};
+
+TEST_F(RegionGraphTest, MultipleBranchesToSameRegionIsNotALoop) {
+  // Both terminators of region 0 branch to region 1.
+  RegionBranchOpInterface op = parse(R"MLIR(
+"cftest.region_graph_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());
+  ) {entries = array<i64: 0>, successors = [array<i64: 1>, array<i64>]}
+    : () -> ()
+  )MLIR");
+  ASSERT_TRUE(op);
+  EXPECT_FALSE(op.isRepetitiveRegion(0));
+  EXPECT_FALSE(op.isRepetitiveRegion(1));
+  EXPECT_FALSE(op.hasLoop());
+}
 
-  EXPECT_FALSE(regionOp.isRepetitiveRegion(0));
-  EXPECT_FALSE(regionOp.isRepetitiveRegion(1));
-  EXPECT_FALSE(regionOp.hasLoop());
+TEST_F(RegionGraphTest, DiamondIsNotALoop) {
+  RegionBranchOpInterface op =
+      parse(/*entries=*/"0", /*successors=*/{"1, 2", "3", "3", ""});
+  ASSERT_TRUE(op);
+  EXPECT_FALSE(op.hasLoop());
 }
 
-TEST(RegionBranchOpInterface, LoopAfterEntryRegion) {
-  const char *ir = R"MLIR(
-"cftest.entry_regions_op"() (
-      {"cftest.dummy_op"() : () -> ()},
-      {"cftest.dummy_op"() : () -> ()}
-  ) {loop} : () -> ()
-  )MLIR";
+TEST_F(RegionGraphTest, LoopAfterEntryRegion) {
+  RegionBranchOpInterface op =
+      parse(/*entries=*/"0", /*successors=*/{"1", "1"});
+  ASSERT_TRUE(op);
+  EXPECT_FALSE(op.isRepetitiveRegion(0));
+  EXPECT_TRUE(op.isRepetitiveRegion(1));
+  EXPECT_TRUE(op.hasLoop());
+}
 
-  DialectRegistry registry;
-  registry.insert<CFTestDialect>();
-  MLIRContext ctx(registry);
+TEST_F(RegionGraphTest, LoopThroughEntryRegion) {
+  RegionBranchOpInterface op =
+      parse(/*entries=*/"0", /*successors=*/{"1", "0"});
+  ASSERT_TRUE(op);
+  EXPECT_TRUE(op.hasLoop());
+}
 
-  OwningOpRef<ModuleOp> module = parseSourceString<ModuleOp>(ir, &ctx);
-  ASSERT_TRUE(module);
-  auto regionOp = cast<RegionBranchOpInterface>(
-      &module->getBody()->getOperations().front());
+TEST_F(RegionGraphTest, UnreachableLoopIsIgnored) {
+  RegionBranchOpInterface op =
+      parse(/*entries=*/"0", /*successors=*/{"1", "", "2"});
+  ASSERT_TRUE(op);
+  EXPECT_TRUE(op.isRepetitiveRegion(2));
+  EXPECT_FALSE(op.hasLoop());
+}
 
-  EXPECT_FALSE(regionOp.isRepetitiveRegion(0));
-  EXPECT_TRUE(regionOp.isRepetitiveRegion(1));
-  EXPECT_TRUE(regionOp.hasLoop());
+TEST_F(RegionGraphTest, OverlappingEntriesAreNotALoop) {
+  // Region 1 is an entry region and is also reachable from entry region 0.
+  for (StringRef entries : {"0, 1", "1, 0"}) {
+    RegionBranchOpInterface op = parse(entries, /*successors=*/{"1", ""});
+    ASSERT_TRUE(op);
+    EXPECT_FALSE(op.hasLoop()) << "entries = " << entries.str();
+  }
 }



More information about the Mlir-commits mailing list