[MLIR] Fail instead of asserting while computing memref strides

[bondhugula]

Summary

Fail getStridesAndOffset utility instead of asserting during an integer
overflow while computing memref lowering strides. The utility is already
designed to fail in two other scenarios. Update the underlying
makeCanonicalStridedLayoutExpr to fail instead of assert. Asserting
leads to a worse user experience in the conversions layered on top.

[bondhugula]

Remove stray change.

[bondhugula]

Despite this change, LLVM type conversion's convertMemRefType would still assert because it asserts on finding a valid strided type; but this change still makes the situation better.

[ftynse]

mlir/lib/IR/BuiltinTypes.cpp

992–997

This is checking for the result of undefined behavior (singled integer overflow). I see it was checking that before, but let's not perpetuate this. If we actually want to detect overflow, let's use APInt with quad bitwidth for multiplication and see if the result fits.

[bondhugula]

mlir/lib/IR/BuiltinTypes.cpp

992–997

Yes, but that's not the purpose of this patch. That can be addressed in a separate patch. This is strictly better on the handling part.

[ftynse]

mlir/lib/IR/BuiltinTypes.cpp

992–997

Given that an undefined behavior happens in this flow, there is no guarantee that the error message will be emitted. The compiler may optimize away the branch. This may result in a segfault elsewhere, all depending on the compiler version and optimization flags. I don't see how that is better. We cannot even test reliably.

[bondhugula]

mlir/lib/IR/BuiltinTypes.cpp

992–997

Wouldn't it have been worse with the assert? (since you won't have it on the release no- assert-build.) This patch is just exposing the issue and keeping the condition ditto. In fact, not exposing this would have more likely perpetuated it. I won't be changing the condition itself and it should be changed in a separate commit. I can rebase on top if someone else wants to fix that. (From the history, the safety check was added by @aartbik.)

[ftynse]

I see asserts as messages for developers that they didn't handle preconditions properly and that the program would eventually crash. A debugging aid that may or may not be there. On the other hand, diagnostics are user-visible messages that should remain consistent across platforms. That's why we have an implicit convention of adding a test for every diagnostics, which would be problematic here. Diagnostics also typically indicate a problem with the IR the user sent to the compiler that the user can act on. While here it is likely that the overflow may be caused by some input IR, there is no indication whether it is indeed the case and how the user can remedy the problem.

[bondhugula]

In D139070#3963711, @ftynse wrote:

I see asserts as messages for developers that they didn't handle preconditions properly and that the program would eventually crash. A debugging aid that may or may not be there. On the other hand, diagnostics are user-visible messages that should remain consistent across platforms. That's why we have an implicit convention of adding a test for every diagnostics, which would be problematic here. Diagnostics also typically indicate a problem with the IR the user sent to the compiler that the user can act on. While here it is likely that the overflow may be caused by some input IR, there is no indication whether it is indeed the case and how the user can remedy the problem.

I can wait until the condition is properly fixed, after which this can be rebased.

mlir/include/mlir/IR/BuiltinTypes.h

 ///         `s0*exprs[0] + 5*exprs[1] + exprs[2]`.
 ///   - memref<3x4x?xf32> has canonical stride expression
 ///         `s1*exprs[0] + s0*exprs[1] + exprs[2]`.
 AffineExpr makeCanonicalStridedLayoutExpr(ArrayRef<int64_t> sizes,
                                           ArrayRef<AffineExpr> exprs,
                                           MLIRContext *context);
 
 /// Return the result of makeCanonicalStrudedLayoutExpr for the common case
-/// where `exprs` is {d0, d1, .., d_(sizes.size()-1)}
+/// where `exprs` is {d0, d1, .., d_(sizes.size()-1)}. Returns null if there
+/// is an integer overflow during computation of the expression (i.e., 64-bits
+/// aren't enough to store the strides).
 AffineExpr makeCanonicalStridedLayoutExpr(ArrayRef<int64_t> sizes,
                                           MLIRContext *context);
 
 /// Return true if the layout for `t` is compatible with strided semantics.
 bool isStrided(MemRefType t);
 
 } // namespace mlir
 
 #endif // MLIR_IR_BUILTINTYPES_H

mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp

LLVM_DEBUG(llvm::dbgs() << "Checking for dependence at depth: "
                           << Twine(loopDepth) << " between:\n";);
   LLVM_DEBUG(srcAccess.opInst->dump(););
   LLVM_DEBUG(dstAccess.opInst->dump(););
 
   // Return 'NoDependence' if these accesses do not access the same memref.
   if (srcAccess.memref != dstAccess.memref)
     return DependenceResult::NoDependence;
 
+  // We can't analyze further if the ops lie in different affine scopes.
+  if (getAffineScope(srcAccess.opInst) != getAffineScope(dstAccess.opInst))
+    return DependenceResult::Failure;
+
   // Return 'NoDependence' if one of these accesses is not an
   // AffineWriteOpInterface.
   if (!allowRAR && !isa<AffineWriteOpInterface>(srcAccess.opInst) &&
       !isa<AffineWriteOpInterface>(dstAccess.opInst))
     return DependenceResult::NoDependence;
 
   // We can't analyze further if the ops lie in different affine scopes.
   if (getAffineScope(srcAccess.opInst) != getAffineScope(dstAccess.opInst))

mlir/lib/IR/BuiltinTypes.cpp

static LogicalResult getStridesAndOffset(MemRefType t,
 
   // Canonical case for empty map.
   if (m.isIdentity()) {
     // 0-D corner case, offset is already 0.
     if (t.getRank() == 0)
       return success();
     auto stridedExpr =
         makeCanonicalStridedLayoutExpr(t.getShape(), t.getContext());
+    if (!stridedExpr)
+      return failure();
     if (succeeded(extractStrides(stridedExpr, one, strides, offset)))
       return success();
     assert(false && "unexpected failure: extract strides in canonical layout");
   }
 
   // Non-canonical case requires more work.
   auto stridedExpr =
       simplifyAffineExpr(m.getResult(0), m.getNumDims(), m.getNumSymbols());
AffineExpr mlir::makeCanonicalStridedLayoutExpr(ArrayRef<int64_t> sizes,
   for (auto en : llvm::zip(llvm::reverse(exprs), llvm::reverse(sizes))) {
     int64_t size = std::get<1>(en);
     AffineExpr dimExpr = std::get<0>(en);
     AffineExpr stride = dynamicPoisonBit
                             ? getAffineSymbolExpr(nSymbols++, context)
                             : getAffineConstantExpr(runningSize, context);
     expr = expr ? expr + dimExpr * stride : dimExpr * stride;
     if (size > 0) {
       runningSize *= size;
-      assert(runningSize > 0 && "integer overflow in size computation");
+      if (runningSize <= 0) {
+        emitError(UnknownLoc::get(context),
+                  "integer overflow during size computation");
+        return AffineExpr();
+      }
     } else {
       dynamicPoisonBit = true;
     }
   }
   return simplifyAffineExpr(expr, numDims, nSymbols);
 }
 
 AffineExpr mlir::makeCanonicalStridedLayoutExpr(ArrayRef<int64_t> sizes,