[Mlir-commits] [mlir] [MLIR][NFC] Speed up is valid symbol check (PR #154924)

[William Moses]

================
@@ -445,42 +460,35 @@ bool mlir::affine::isValidSymbol(Value value, Region *region) {
     return false;
 
   // A top-level value is a valid symbol.
-  if (region && ::isTopLevelValue(value, region))
+  if (region && isTopLevelValueOrAbove(value, region))
     return true;
 
   auto *defOp = value.getDefiningOp();
-  if (!defOp) {
-    // A block argument that is not a top-level value is a valid symbol if it
-    // dominates region's parent op.
-    Operation *regionOp = region ? region->getParentOp() : nullptr;
-    if (regionOp && !regionOp->hasTrait<OpTrait::IsIsolatedFromAbove>())
-      if (auto *parentOpRegion = region->getParentOp()->getParentRegion())
-        return isValidSymbol(value, parentOpRegion);
+  if (!defOp)
     return false;
-  }
 
   // Constant operation is ok.
   Attribute operandCst;
   if (matchPattern(defOp, m_Constant(&operandCst)))
     return true;
 
   // `Pure` operation that whose operands are valid symbolic identifiers.
-  if (isPure(defOp) && llvm::all_of(defOp->getOperands(), [&](Value operand) {
-        return affine::isValidSymbol(operand, region);
-      })) {
-    return true;
+  if (isPure(defOp)) {
+    bool allValid = true;
+    for (auto operand : defOp->getOperands()) {
+      if (!affine::isValidSymbol(operand, region)) {
+        allValid = false;
+        break;
+      }
+    }
+    if (allValid)
+      return true;
   }
 
   // Dim op results could be valid symbols at any level.
   if (auto dimOp = dyn_cast<ShapedDimOpInterface>(defOp))
     return isDimOpValidSymbol(dimOp, region);
 
-  // Check for values dominating `region`'s parent op.
-  Operation *regionOp = region ? region->getParentOp() : nullptr;
-  if (regionOp && !regionOp->hasTrait<OpTrait::IsIsolatedFromAbove>())
-    if (auto *parentRegion = region->getParentOp()->getParentRegion())
-      return isValidSymbol(value, parentRegion);
----------------
wsmoses wrote:

it is intending to be NFC, my rough thoughts/logic below.

Essentially the code is

```
if (is top level in region and/or value is constant and/or is pure) {
  return true
}

if region.parent != null and if is symbol in parent region
  return true ;// we are a valid symbol in current region
```

the check for "value is constant and/or is pure" is invariant to the region specifier, and has to be done for each recursive region check. So essentially we rewrite the `is top level in region` to be `is top level in region or above` and do the recursive check in a single call there.

said another way, the current code first checks if the value is a symbol in the current region (either is constant, top level symbol in region, etc). if it is not, the current code recurses if the region being checked has a parent, and returns that it is a valid symbol in this region if it is a valid symbol in the parent

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