[MLIR][Affine] Fix addInductionVarOrTerminalSymbol to return status

[bondhugula]

Fixes: #64287

This method is failable on valid IR and we should've been returning
failure instead of asserting, and checking status at its users.

[llvmbot]

@llvm/pr-subscribers-mlir-affine

@llvm/pr-subscribers-mlir

Author: Uday Bondhugula (bondhugula)

Changes

Fixes: #64287

This method is failable on valid IR and we should've been returning
failure instead of asserting, and checking status at its users.

---

Full diff: https://github.com/llvm/llvm-project/pull/129476.diff

3 Files Affected:

• (modified) mlir/include/mlir/Dialect/Affine/Analysis/AffineStructures.h (+8-5)
• (modified) mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp (+22-11)
• (modified) mlir/lib/Dialect/Affine/Analysis/Utils.cpp (+2-1)

diff --git a/mlir/include/mlir/Dialect/Affine/Analysis/AffineStructures.h b/mlir/include/mlir/Dialect/Affine/Analysis/AffineStructures.h
index 707eec779aa58..e6596f2e010dc 100644
--- a/mlir/include/mlir/Dialect/Affine/Analysis/AffineStructures.h
+++ b/mlir/include/mlir/Dialect/Affine/Analysis/AffineStructures.h
@@ -106,11 +106,14 @@ class FlatAffineValueConstraints : public FlatLinearValueConstraints {
 
   /// Add the specified values as a dim or symbol var depending on its nature,
   /// if it already doesn't exist in the system. `val` has to be either a
-  /// terminal symbol or a loop IV, i.e., it cannot be the result affine.apply
-  /// of any symbols or loop IVs. The variable is added to the end of the
-  /// existing dims or symbols. Additional information on the variable is
-  /// extracted from the IR and added to the constraint system.
-  void addInductionVarOrTerminalSymbol(Value val);
+  /// terminal symbol or a loop IV, i.e., it cannot be the result of an
+  /// affine.apply of any symbols or loop IVs. Return failure if the addition
+  /// wasn't possible due to the above conditions not being met. This method can
+  /// also fail if the addition of the domain of an affine IV fails. The
+  /// variable is added to the end of the existing dims or symbols. Additional
+  /// information on the variable is extracted from the IR and added to the
+  /// constraint system.
+  LogicalResult addInductionVarOrTerminalSymbol(Value val);
 
   /// Adds slice lower bounds represented by lower bounds in `lbMaps` and upper
   /// bounds in `ubMaps` to each variable in the constraint system which has
diff --git a/mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp b/mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp
index 034484af83b79..efecc5d247e4b 100644
--- a/mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp
+++ b/mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp
@@ -34,28 +34,36 @@ using namespace mlir;
 using namespace affine;
 using namespace presburger;
 
-
-void FlatAffineValueConstraints::addInductionVarOrTerminalSymbol(Value val) {
+LogicalResult
+FlatAffineValueConstraints::addInductionVarOrTerminalSymbol(Value val) {
   if (containsVar(val))
-    return;
+    return success();
 
   // Caller is expected to fully compose map/operands if necessary.
-  assert((isTopLevelValue(val) || isAffineInductionVar(val)) &&
-         "non-terminal symbol / loop IV expected");
+  if (!isTopLevelValue(val) && !isAffineInductionVar(val)) {
+    LLVM_DEBUG(llvm::dbgs()
+               << "only valid terminal symbols and affine IVs supported\n");
+    return failure();
+  }
   // Outer loop IVs could be used in forOp's bounds.
   if (auto loop = getForInductionVarOwner(val)) {
     appendDimVar(val);
-    if (failed(this->addAffineForOpDomain(loop)))
+    if (failed(this->addAffineForOpDomain(loop))) {
       LLVM_DEBUG(
           loop.emitWarning("failed to add domain info to constraint system"));
-    return;
+      return failure();
+    }
+    return success();
   }
+
   if (auto parallel = getAffineParallelInductionVarOwner(val)) {
     appendDimVar(parallel.getIVs());
-    if (failed(this->addAffineParallelOpDomain(parallel)))
+    if (failed(this->addAffineParallelOpDomain(parallel))) {
       LLVM_DEBUG(parallel.emitWarning(
           "failed to add domain info to constraint system"));
-    return;
+      return failure();
+    }
+    return success();
   }
 
   // Add top level symbol.
@@ -63,6 +71,7 @@ void FlatAffineValueConstraints::addInductionVarOrTerminalSymbol(Value val) {
   // Check if the symbol is a constant.
   if (std::optional<int64_t> constOp = getConstantIntValue(val))
     addBound(BoundType::EQ, val, constOp.value());
+  return success();
 }
 
 LogicalResult
@@ -222,8 +231,10 @@ LogicalResult FlatAffineValueConstraints::addBound(BoundType type, unsigned pos,
   fullyComposeAffineMapAndOperands(&map, &operands);
   map = simplifyAffineMap(map);
   canonicalizeMapAndOperands(&map, &operands);
-  for (auto operand : operands)
-    addInductionVarOrTerminalSymbol(operand);
+  for (Value operand : operands) {
+    if (failed(addInductionVarOrTerminalSymbol(operand)))
+      return failure();
+  }
   return addBound(type, pos, computeAlignedMap(map, operands));
 }
 
diff --git a/mlir/lib/Dialect/Affine/Analysis/Utils.cpp b/mlir/lib/Dialect/Affine/Analysis/Utils.cpp
index ba6f045cff408..db9cfc4c23bba 100644
--- a/mlir/lib/Dialect/Affine/Analysis/Utils.cpp
+++ b/mlir/lib/Dialect/Affine/Analysis/Utils.cpp
@@ -1255,7 +1255,8 @@ LogicalResult MemRefRegion::compute(Operation *op, unsigned loopDepth,
   if (sliceState != nullptr) {
     // Add dim and symbol slice operands.
     for (auto operand : sliceState->lbOperands[0]) {
-      cst.addInductionVarOrTerminalSymbol(operand);
+      if (failed(cst.addInductionVarOrTerminalSymbol(operand)))
+        return failure();
     }
     // Add upper/lower bounds from 'sliceState' to 'cst'.
     LogicalResult ret =

[bondhugula]

Obvious fix; fixes filed crash bug. Merging.