[Mlir-commits] [mlir] [mlir][memref] Detect negative `offset` or `size` for `subview` (PR #72059)

llvmlistbot at llvm.org llvmlistbot at llvm.org
Sun Nov 12 09:56:52 PST 2023


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir-memref

Author: Rik Huijzer (rikhuijzer)

<details>
<summary>Changes</summary>

In #<!-- -->71153, the `memref.subview` canonicalizer crashes due to a negative `size` being passed as an operand. During `SubViewOp::verify` this negative `size` is not yet detectable since it is dynamic and only available after constant folding, which happens during the canonicalization passes. As discussed in <https://discourse.llvm.org/t/rfc-more-opfoldresult-and-mixed-indices-in-ops-that-deal-with-shaped-values/72510>, the verifier should not be extended as it should "only verify local aspects of an operation". Furthermore, the discussion talks about possible solutions here, but it seems to me that there is no consensus yet?

This patch proposes to add a basic offset and size check in `SubViewOp::verify` and add a slightly more clear assertion error after the constant folding. Without this assertion, it would crash with the following message:
```
<unknown>:0: error: invalid memref size
Assertion failed: (succeeded(ConcreteT::verify(getDefaultDiagnosticEmitFn(ctx), args...))), function get, file StorageUniquerSupport.h, line 181.
```

Note: this patch only checks `offset` and `size` because `stride` is allowed to be negative (https://github.com/llvm/llvm-project/commit/54d81e49e3b72f6a305891fe169ecd7c6f559223).

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


3 Files Affected:

- (modified) mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp (+20-1) 
- (modified) mlir/lib/Dialect/Tensor/IR/TensorOps.cpp (+1-1) 
- (modified) mlir/test/Dialect/MemRef/invalid.mlir (+16) 


``````````diff
diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
index 215a8f5e7d18be0..86d6f6bf6ad5388 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -2621,6 +2621,15 @@ Type SubViewOp::inferResultType(MemRefType sourceMemRefType,
   dispatchIndexOpFoldResults(offsets, dynamicOffsets, staticOffsets);
   dispatchIndexOpFoldResults(sizes, dynamicSizes, staticSizes);
   dispatchIndexOpFoldResults(strides, dynamicStrides, staticStrides);
+
+  for (int64_t offset : staticOffsets) {
+    if (!ShapedType::isDynamic(offset))
+      assert(offset >= 0 && "expected subview offsets to be non-negative");
+  }
+  for (int64_t size : staticSizes) {
+    if (!ShapedType::isDynamic(size))
+      assert(size >= 0 && "expected subview sizes to be non-negative");
+  }
   return SubViewOp::inferResultType(sourceMemRefType, staticOffsets,
                                     staticSizes, staticStrides);
 }
@@ -2842,8 +2851,18 @@ static LogicalResult produceSubViewErrorMsg(SliceVerificationResult result,
   llvm_unreachable("unexpected subview verification result");
 }
 
-/// Verifier for SubViewOp.
 LogicalResult SubViewOp::verify() {
+  for (int64_t offset : getStaticOffsets()) {
+    if (offset < 0 && !ShapedType::isDynamic(offset))
+      return emitError("expected subview offsets to be non-negative, but got ")
+             << offset;
+  }
+  for (int64_t size : getStaticSizes()) {
+    if (size < 0 && !ShapedType::isDynamic(size))
+      return emitError("expected subview sizes to be non-negative, but got ")
+             << size;
+  }
+
   MemRefType baseType = getSourceType();
   MemRefType subViewType = getType();
 
diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
index 6fc45379111fc34..ab915c0e786aeb5 100644
--- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
+++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
@@ -1242,7 +1242,7 @@ struct StaticTensorGenerate : public OpRewritePattern<GenerateOp> {
 
     for (int64_t newdim : newShape) {
       // This check also occurs in the verifier, but we need it here too
-      // since intermediate passes may have some replaced dynamic dimensions
+      // since intermediate passes may have replaced some dynamic dimensions
       // by constants.
       if (newdim < 0 && !ShapedType::isDynamic(newdim))
         return failure();
diff --git a/mlir/test/Dialect/MemRef/invalid.mlir b/mlir/test/Dialect/MemRef/invalid.mlir
index cb5977e302a993f..38c0bcc3f2491c2 100644
--- a/mlir/test/Dialect/MemRef/invalid.mlir
+++ b/mlir/test/Dialect/MemRef/invalid.mlir
@@ -611,6 +611,22 @@ func.func @invalid_view(%arg0 : index, %arg1 : index, %arg2 : index) {
 
 // -----
 
+func.func @invalid_subview(%input: memref<4x1024xf32>) -> memref<2x256xf32, strided<[1024, 1], offset: 2304>> {
+  // expected-error at +1 {{expected subview offsets to be non-negative, but got -1}}
+  %0 = memref.subview %input[-1, 256] [2, 256] [1, 1] : memref<4x1024xf32> to memref<2x256xf32, strided<[1024, 1], offset: 2304>>
+  return %0 : memref<2x256xf32, strided<[1024, 1], offset: 2304>>
+}
+
+// -----
+
+func.func @invalid_subview(%input: memref<4x1024xf32>) -> memref<2x256xf32, strided<[1024, 1], offset: 2304>> {
+  // expected-error at +1 {{expected subview sizes to be non-negative, but got -1}}
+  %0 = memref.subview %input[2, 256] [-1, 256] [1, 1] : memref<4x1024xf32> to memref<2x256xf32, strided<[1024, 1], offset: 2304>>
+  return %0 : memref<2x256xf32, strided<[1024, 1], offset: 2304>>
+}
+
+// -----
+
 func.func @invalid_subview(%arg0 : index, %arg1 : index, %arg2 : index) {
   %0 = memref.alloc() : memref<8x16x4xf32>
   // expected-error at +1 {{expected mixed offsets rank to match mixed sizes rank (2 vs 3) so the rank of the result type is well-formed}}

``````````

</details>


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


More information about the Mlir-commits mailing list