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

[Mehdi Amini]

================
@@ -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);
+           });
+  });
----------------
joker-eph wrote:

This looks costly to me...
I asked an agent to suggest something about the performance aspect here:

> The stop condition is evaluated on every worklist  pop before the visited check, and the new lambda runs a full isRegionReachable(r, r) each time, so hasLoop is  O(T·(B+T)) in region-successor edges T rather than O(B+T). For every in-tree op reaching this call T is zero  (terminators branch only to the parent), so cost is unchanged. An out-of-tree op with many region terminators  targeting other regions would see a quadratic blowup. If the author wants to tighten it cheaply: capture a  per-call SmallBitVector in the lambda to run the inner check at most once per region, giving O(R·(B+T)).

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