[Mlir-commits] [mlir] [MLIR][Tensor] Fix out-of-bounds FoldEmptyTensorWithDimOp crash #111270 (PR #112196)

[llvmlistbot at llvm.org]

================
@@ -979,7 +979,9 @@ struct FoldEmptyTensorWithDimOp : public OpRewritePattern<DimOp> {
     auto emptyTensorOp = dimOp.getSource().getDefiningOp<EmptyOp>();
     if (!emptyTensorOp || !maybeConstantIndex)
       return failure();
-    if (!emptyTensorOp.getType().isDynamicDim(*maybeConstantIndex))
+    auto emptyTensorType = emptyTensorOp.getType();
----------------
brod4910 wrote:

@MaheshRavishankar after further inspection, I found this little excerpt in the docs about the IR verifier: https://mlir.llvm.org/getting_started/DeveloperGuide/#ir-verifier that states statically verifying an expression such as this one in the verifier should be avoided.


> While it is encouraged to verify as much invariants as possible in order to catch bugs during development as soon as possible, there is some important aspect to keep in mind. In particular a common point of confusion is about how to handle “undefined behavior” cases. For example the tensor.dim operation:

```mlir
// Returns the dimension of %A indexed by %dim.
%y = tensor.dim %A, %dim : memref<4x?xf32>
```

> The %dim indicates what dimension to return. If the dimension index is out of bounds, the behavior is undefined. What about:

```mlir
%ten = arith.constant 10 : index
%y = tensor.dim %A, %ten : memref<4x?xf32>
```

> We have unambiguously a violation of the spec here, and we can statically verify it. However this is not the kind of invariants to enforce in a verifier because it relies on non-local properties, which makes the design of the compiler much less flexible.

Implementing the change in the verifier seems to have other implications in the lit tests here in these files:
`Dialect/MemRef/resolve-dim-ops.mlir` and  `Dialect/Tensor/canonicalize.mlir`. The test cases seem to suggest that even though the indices are statically known they should still pass. 

When implementing the change in the verifier the test cases fail. For example in the function `dim_out_of_bounds_2` in `resolve-dim-ops.mlir`:

```
# .---command stderr------------
# | within split at /llvm-project/mlir/test/Dialect/MemRef/resolve-dim-ops.mlir:13 offset :13:8: error: 'tensor.dim' op out-of-range access, attempted to access index 7 but valid range is [0, 1].
# |   %0 = tensor.dim %alloc, %idx : tensor<?x?xf32>
# |        ^
# | within split at /llvm-project/mlir/test/Dialect/MemRef/resolve-dim-ops.mlir:13 offset :13:8: note: see current operation: %3 = "tensor.dim"(%2, %0) : (tensor<?x?xf32>, index) -> index
# `-----------------------------
# error: command failed with exit status: 1
```

These tests previously passed without the verifier change. Not sure what to make of this so your thoughts are much appreciated!

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