[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:

> > `if is symbol in parent region`
> 
> This is the condition I mentioned, which does not match the API doc somehow (line 438 I mentioned before).
> 
> I don't see where you're checking on this "`if is symbol in parent region`" after your change? I don't follow when you write:
> 

So one property of isSymbolInRegion is that (assuming not isolated from above), a symbol in the parent region is definitionally a symbol in the current region.

isTopLevelValueOrAbove now checks the in parent region too, negating the need for an explicit recursion for that case. 

For this part of the code:
```
  if (isPure(defOp) && llvm::all_of(defOp->getOperands(), [&](Value operand) {
        return affine::isValidSymbol(operand, region);
      })) {
    return true;
  }
```
we no longer explicitly check isValidSymbol(operand, parentRegion). But if isValidSymbol(operand, parentRegion) is true, isValidSymbol(operand, region) must also definitionally be true, so we don't need the extra check. Also ispure doesn't consider parent region so obviously doesn't need a redundant call.

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