[flang] [clang] [clang-tools-extra] [mlir] [compiler-rt] [llvm] [mlir][memref] Detect negative `offset` or `size` for `subview` (PR #72059)

Rik Huijzer via cfe-commits cfe-commits at lists.llvm.org
Mon Nov 13 03:01:23 PST 2023


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

>From 8f90ae9113229faf5ab4b859e5c70f23853f4db8 Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Sat, 11 Nov 2023 20:43:31 +0100
Subject: [PATCH 1/9] [mlir][memref] Verify subview offsets and sizes

---
 mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp | 12 +++++++++++-
 mlir/test/Dialect/MemRef/invalid.mlir    | 16 ++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
index 215a8f5e7d18be0..dc3e04be83fc1c9 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -2842,8 +2842,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 < 1 && !ShapedType::isDynamic(size))
+      return emitError("expected subview sizes to be positive, but got ")
+             << size;
+  }
+
   MemRefType baseType = getSourceType();
   MemRefType subViewType = getType();
 
diff --git a/mlir/test/Dialect/MemRef/invalid.mlir b/mlir/test/Dialect/MemRef/invalid.mlir
index cb5977e302a993f..17f5fbea3bda17a 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 positive, but got 0}}
+  %0 = memref.subview %input[2, 256] [0, 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}}

>From 32adf97c8d19e29e4673c772bd190c0eb0b413e9 Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Sun, 12 Nov 2023 09:10:00 +0100
Subject: [PATCH 2/9] Allow zero sizes

---
 mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp | 5 +++--
 mlir/test/Dialect/MemRef/invalid.mlir    | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
index dc3e04be83fc1c9..a24467a321a70c5 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -2849,8 +2849,8 @@ LogicalResult SubViewOp::verify() {
              << offset;
   }
   for (int64_t size : getStaticSizes()) {
-    if (size < 1 && !ShapedType::isDynamic(size))
-      return emitError("expected subview sizes to be positive, but got ")
+    if (size < 0 && !ShapedType::isDynamic(size))
+      return emitError("expected subview sizes to be non-negative, but got ")
              << size;
   }
 
@@ -3103,6 +3103,7 @@ struct SubViewReturnTypeCanonicalizer {
   MemRefType operator()(SubViewOp op, ArrayRef<OpFoldResult> mixedOffsets,
                         ArrayRef<OpFoldResult> mixedSizes,
                         ArrayRef<OpFoldResult> mixedStrides) {
+
     // Infer a memref type without taking into account any rank reductions.
     MemRefType nonReducedType = cast<MemRefType>(SubViewOp::inferResultType(
         op.getSourceType(), mixedOffsets, mixedSizes, mixedStrides));
diff --git a/mlir/test/Dialect/MemRef/invalid.mlir b/mlir/test/Dialect/MemRef/invalid.mlir
index 17f5fbea3bda17a..38c0bcc3f2491c2 100644
--- a/mlir/test/Dialect/MemRef/invalid.mlir
+++ b/mlir/test/Dialect/MemRef/invalid.mlir
@@ -620,8 +620,8 @@ func.func @invalid_subview(%input: memref<4x1024xf32>) -> memref<2x256xf32, stri
 // -----
 
 func.func @invalid_subview(%input: memref<4x1024xf32>) -> memref<2x256xf32, strided<[1024, 1], offset: 2304>> {
-  // expected-error at +1 {{expected subview sizes to be positive, but got 0}}
-  %0 = memref.subview %input[2, 256] [0, 256] [1, 1] : memref<4x1024xf32> to 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>>
 }
 

>From 30fc408fec59c656c90b3bd927b1cd6e93b18e3d Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Sun, 12 Nov 2023 15:28:55 +0100
Subject: [PATCH 3/9] Catch negative size earlier

This now does catch the negative size inside the interface so that an
`op.emitError` can be thrown. That works, but then continues to
return an empty result?

Instead, the interface can probably be refactored first because it's
very restrictive in its current form.
---
 mlir/include/mlir/Interfaces/ViewLikeInterface.h | 14 ++++++++++++++
 mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp         | 15 +++++++++++++++
 mlir/lib/Dialect/Tensor/IR/TensorOps.cpp         |  5 ++++-
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/mlir/include/mlir/Interfaces/ViewLikeInterface.h b/mlir/include/mlir/Interfaces/ViewLikeInterface.h
index a114e9af126f112..80d5b656cccb066 100644
--- a/mlir/include/mlir/Interfaces/ViewLikeInterface.h
+++ b/mlir/include/mlir/Interfaces/ViewLikeInterface.h
@@ -19,6 +19,7 @@
 #include "mlir/IR/BuiltinTypes.h"
 #include "mlir/IR/OpImplementation.h"
 #include "mlir/IR/PatternMatch.h"
+#include <_types/_uint64_t.h>
 
 namespace mlir {
 
@@ -72,6 +73,19 @@ class OpWithOffsetSizesAndStridesConstantArgumentFolder final
         failed(foldDynamicIndexList(mixedStrides)))
       return failure();
 
+    SmallVector<int64_t> staticOffsets, staticSizes, staticStrides;
+    SmallVector<Value> dynamicOffsets, dynamicSizes, dynamicStrides;
+    dispatchIndexOpFoldResults(mixedOffsets, dynamicOffsets, staticOffsets);
+    dispatchIndexOpFoldResults(mixedSizes, dynamicSizes, staticSizes);
+    dispatchIndexOpFoldResults(mixedStrides, dynamicStrides, staticStrides);
+
+    for (int64_t size : staticSizes) {
+      if (size < 0 && !ShapedType::isDynamic(size)) {
+        return op.emitError("expected non-negative size, but got ")
+               << size;;
+      }
+    }
+
     // Create the new op in canonical form.
     auto resultType =
         ResultTypeFn()(op, mixedOffsets, mixedSizes, mixedStrides);
diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
index a24467a321a70c5..477f9e050cafd1b 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -2843,6 +2843,8 @@ static LogicalResult produceSubViewErrorMsg(SliceVerificationResult result,
 }
 
 LogicalResult SubViewOp::verify() {
+  llvm::outs() << "SubViewOp::verify\n";
+
   for (int64_t offset : getStaticOffsets()) {
     if (offset < 0 && !ShapedType::isDynamic(offset))
       return emitError("expected subview offsets to be non-negative, but got ")
@@ -3103,6 +3105,19 @@ struct SubViewReturnTypeCanonicalizer {
   MemRefType operator()(SubViewOp op, ArrayRef<OpFoldResult> mixedOffsets,
                         ArrayRef<OpFoldResult> mixedSizes,
                         ArrayRef<OpFoldResult> mixedStrides) {
+    SmallVector<int64_t> staticOffsets, staticSizes, staticStrides;
+    SmallVector<Value> dynamicOffsets, dynamicSizes, dynamicStrides;
+    dispatchIndexOpFoldResults(mixedOffsets, dynamicOffsets, staticOffsets);
+    dispatchIndexOpFoldResults(mixedSizes, dynamicSizes, staticSizes);
+    dispatchIndexOpFoldResults(mixedStrides, dynamicStrides, staticStrides);
+
+    for (int64_t size : staticSizes) {
+      if (size < 0 && !ShapedType::isDynamic(size)) {
+        llvm::dbgs() << "expected subview sizes to be non-negative, but got "
+                     << size << "\n";
+        return {};
+      }
+    }
 
     // Infer a memref type without taking into account any rank reductions.
     MemRefType nonReducedType = cast<MemRefType>(SubViewOp::inferResultType(
diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
index 6fc45379111fc34..507123463ec941a 100644
--- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
+++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
@@ -1163,6 +1163,8 @@ static void operandsAndShape(TensorType resultType,
 }
 
 LogicalResult GenerateOp::verify() {
+  llvm::outs() << "GenerateOp::verify()\n";
+
   // Ensure that the tensor type has as many dynamic dimensions as are
   // specified by the operands.
   RankedTensorType resultType = llvm::cast<RankedTensorType>(getType());
@@ -1173,6 +1175,7 @@ LogicalResult GenerateOp::verify() {
   SmallVector<Value> newOperands;
   SmallVector<int64_t> newShape;
   operandsAndShape(resultType, getDynamicExtents(), newOperands, newShape);
+
   for (int64_t newdim : newShape) {
     if (newdim < 0 && !ShapedType::isDynamic(newdim))
       return emitError("tensor dimensions must be non-negative");
@@ -1242,7 +1245,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();

>From 7323aa03ffe2d541fb0e555755edc11c132e3f72 Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Sun, 12 Nov 2023 17:48:11 +0100
Subject: [PATCH 4/9] Add assertion in outer `inferResultType

---
 .../mlir/Interfaces/ViewLikeInterface.h       | 13 ---------
 mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp      | 29 +++++++++----------
 mlir/lib/Dialect/Tensor/IR/TensorOps.cpp      |  2 --
 3 files changed, 13 insertions(+), 31 deletions(-)

diff --git a/mlir/include/mlir/Interfaces/ViewLikeInterface.h b/mlir/include/mlir/Interfaces/ViewLikeInterface.h
index 80d5b656cccb066..7867fb2429ca3df 100644
--- a/mlir/include/mlir/Interfaces/ViewLikeInterface.h
+++ b/mlir/include/mlir/Interfaces/ViewLikeInterface.h
@@ -73,19 +73,6 @@ class OpWithOffsetSizesAndStridesConstantArgumentFolder final
         failed(foldDynamicIndexList(mixedStrides)))
       return failure();
 
-    SmallVector<int64_t> staticOffsets, staticSizes, staticStrides;
-    SmallVector<Value> dynamicOffsets, dynamicSizes, dynamicStrides;
-    dispatchIndexOpFoldResults(mixedOffsets, dynamicOffsets, staticOffsets);
-    dispatchIndexOpFoldResults(mixedSizes, dynamicSizes, staticSizes);
-    dispatchIndexOpFoldResults(mixedStrides, dynamicStrides, staticStrides);
-
-    for (int64_t size : staticSizes) {
-      if (size < 0 && !ShapedType::isDynamic(size)) {
-        return op.emitError("expected non-negative size, but got ")
-               << size;;
-      }
-    }
-
     // Create the new op in canonical form.
     auto resultType =
         ResultTypeFn()(op, mixedOffsets, mixedSizes, mixedStrides);
diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
index 477f9e050cafd1b..6f2f08897784569 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -2621,6 +2621,19 @@ Type SubViewOp::inferResultType(MemRefType sourceMemRefType,
   dispatchIndexOpFoldResults(offsets, dynamicOffsets, staticOffsets);
   dispatchIndexOpFoldResults(sizes, dynamicSizes, staticSizes);
   dispatchIndexOpFoldResults(strides, dynamicStrides, staticStrides);
+
+  // Double-check the offsets, sizes, and strides after constant folding.
+  // This allows throwing a more informative assertion message than
+  // what would be thrown at a later point.
+  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);
 }
@@ -2843,8 +2856,6 @@ static LogicalResult produceSubViewErrorMsg(SliceVerificationResult result,
 }
 
 LogicalResult SubViewOp::verify() {
-  llvm::outs() << "SubViewOp::verify\n";
-
   for (int64_t offset : getStaticOffsets()) {
     if (offset < 0 && !ShapedType::isDynamic(offset))
       return emitError("expected subview offsets to be non-negative, but got ")
@@ -3105,20 +3116,6 @@ struct SubViewReturnTypeCanonicalizer {
   MemRefType operator()(SubViewOp op, ArrayRef<OpFoldResult> mixedOffsets,
                         ArrayRef<OpFoldResult> mixedSizes,
                         ArrayRef<OpFoldResult> mixedStrides) {
-    SmallVector<int64_t> staticOffsets, staticSizes, staticStrides;
-    SmallVector<Value> dynamicOffsets, dynamicSizes, dynamicStrides;
-    dispatchIndexOpFoldResults(mixedOffsets, dynamicOffsets, staticOffsets);
-    dispatchIndexOpFoldResults(mixedSizes, dynamicSizes, staticSizes);
-    dispatchIndexOpFoldResults(mixedStrides, dynamicStrides, staticStrides);
-
-    for (int64_t size : staticSizes) {
-      if (size < 0 && !ShapedType::isDynamic(size)) {
-        llvm::dbgs() << "expected subview sizes to be non-negative, but got "
-                     << size << "\n";
-        return {};
-      }
-    }
-
     // Infer a memref type without taking into account any rank reductions.
     MemRefType nonReducedType = cast<MemRefType>(SubViewOp::inferResultType(
         op.getSourceType(), mixedOffsets, mixedSizes, mixedStrides));
diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
index 507123463ec941a..3c4fc1b58ed26cf 100644
--- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
+++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
@@ -1163,8 +1163,6 @@ static void operandsAndShape(TensorType resultType,
 }
 
 LogicalResult GenerateOp::verify() {
-  llvm::outs() << "GenerateOp::verify()\n";
-
   // Ensure that the tensor type has as many dynamic dimensions as are
   // specified by the operands.
   RankedTensorType resultType = llvm::cast<RankedTensorType>(getType());

>From 1859a9b3e0969202d7f97c78ed083f5f179e2632 Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Sun, 12 Nov 2023 17:52:53 +0100
Subject: [PATCH 5/9] Update comment

---
 mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
index 6f2f08897784569..69ca8275e736636 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -2622,9 +2622,7 @@ Type SubViewOp::inferResultType(MemRefType sourceMemRefType,
   dispatchIndexOpFoldResults(sizes, dynamicSizes, staticSizes);
   dispatchIndexOpFoldResults(strides, dynamicStrides, staticStrides);
 
-  // Double-check the offsets, sizes, and strides after constant folding.
-  // This allows throwing a more informative assertion message than
-  // what would be thrown at a later point.
+  // TODO: Handle these situations gracefully in the canonicalizer.
   for (int64_t offset : staticOffsets) {
     if (!ShapedType::isDynamic(offset))
       assert(offset >= 0 && "expected subview offsets to be non-negative");
@@ -2633,7 +2631,6 @@ Type SubViewOp::inferResultType(MemRefType sourceMemRefType,
     if (!ShapedType::isDynamic(size))
       assert(size >= 0 && "expected subview sizes to be non-negative");
   }
-
   return SubViewOp::inferResultType(sourceMemRefType, staticOffsets,
                                     staticSizes, staticStrides);
 }

>From 68778fb9e1b7bc2dec5d0609c0adbb5b0a1875e1 Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Sun, 12 Nov 2023 17:58:21 +0100
Subject: [PATCH 6/9] Revert some changes

---
 mlir/include/mlir/Interfaces/ViewLikeInterface.h | 1 -
 mlir/lib/Dialect/Tensor/IR/TensorOps.cpp         | 1 -
 2 files changed, 2 deletions(-)

diff --git a/mlir/include/mlir/Interfaces/ViewLikeInterface.h b/mlir/include/mlir/Interfaces/ViewLikeInterface.h
index 7867fb2429ca3df..a114e9af126f112 100644
--- a/mlir/include/mlir/Interfaces/ViewLikeInterface.h
+++ b/mlir/include/mlir/Interfaces/ViewLikeInterface.h
@@ -19,7 +19,6 @@
 #include "mlir/IR/BuiltinTypes.h"
 #include "mlir/IR/OpImplementation.h"
 #include "mlir/IR/PatternMatch.h"
-#include <_types/_uint64_t.h>
 
 namespace mlir {
 
diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
index 3c4fc1b58ed26cf..16e4c99a82a5615 100644
--- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
+++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
@@ -1240,7 +1240,6 @@ struct StaticTensorGenerate : public OpRewritePattern<GenerateOp> {
     SmallVector<Value> newOperands;
     SmallVector<int64_t> newShape;
     operandsAndShape(resultType, dynamicExtents, newOperands, newShape);
-
     for (int64_t newdim : newShape) {
       // This check also occurs in the verifier, but we need it here too
       // since intermediate passes may have replaced some dynamic dimensions

>From 6ef2ddaf31a5acea6a65e46a07483da2cf2b99a4 Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Sun, 12 Nov 2023 17:59:04 +0100
Subject: [PATCH 7/9] Revert some changes

---
 mlir/lib/Dialect/Tensor/IR/TensorOps.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
index 16e4c99a82a5615..ab915c0e786aeb5 100644
--- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
+++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
@@ -1173,7 +1173,6 @@ LogicalResult GenerateOp::verify() {
   SmallVector<Value> newOperands;
   SmallVector<int64_t> newShape;
   operandsAndShape(resultType, getDynamicExtents(), newOperands, newShape);
-
   for (int64_t newdim : newShape) {
     if (newdim < 0 && !ShapedType::isDynamic(newdim))
       return emitError("tensor dimensions must be non-negative");
@@ -1240,6 +1239,7 @@ struct StaticTensorGenerate : public OpRewritePattern<GenerateOp> {
     SmallVector<Value> newOperands;
     SmallVector<int64_t> newShape;
     operandsAndShape(resultType, dynamicExtents, newOperands, newShape);
+
     for (int64_t newdim : newShape) {
       // This check also occurs in the verifier, but we need it here too
       // since intermediate passes may have replaced some dynamic dimensions

>From 4036cd14622b16d260764d5f6b7d7090bf10a775 Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Sun, 12 Nov 2023 18:49:45 +0100
Subject: [PATCH 8/9] Remove comment

---
 mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
index 69ca8275e736636..86d6f6bf6ad5388 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -2622,7 +2622,6 @@ Type SubViewOp::inferResultType(MemRefType sourceMemRefType,
   dispatchIndexOpFoldResults(sizes, dynamicSizes, staticSizes);
   dispatchIndexOpFoldResults(strides, dynamicStrides, staticStrides);
 
-  // TODO: Handle these situations gracefully in the canonicalizer.
   for (int64_t offset : staticOffsets) {
     if (!ShapedType::isDynamic(offset))
       assert(offset >= 0 && "expected subview offsets to be non-negative");

>From e4e40f2529e54c28e30f511c420e9f80eceafc65 Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Mon, 13 Nov 2023 12:01:04 +0100
Subject: [PATCH 9/9] Move logic to `verifyOffsetSizeAndStrideOp`

---
 mlir/include/mlir/Interfaces/ViewLikeInterface.td |  1 +
 mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp          | 11 -----------
 mlir/lib/Interfaces/ViewLikeInterface.cpp         | 11 +++++++++++
 mlir/test/Dialect/MemRef/invalid.mlir             |  4 ++--
 4 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/mlir/include/mlir/Interfaces/ViewLikeInterface.td b/mlir/include/mlir/Interfaces/ViewLikeInterface.td
index 8029380a2c8f1dc..ea5bb1b5ac48535 100644
--- a/mlir/include/mlir/Interfaces/ViewLikeInterface.td
+++ b/mlir/include/mlir/Interfaces/ViewLikeInterface.td
@@ -56,6 +56,7 @@ def OffsetSizeAndStrideOpInterface : OpInterface<"OffsetSizeAndStrideOpInterface
          for each dynamic offset (resp. size, stride).
       5. `offsets`, `sizes` and `strides` operands are specified in this order
          at operand index starting at `getOffsetSizeAndStrideStartOperandIndex`.
+      6. `offsets` and `sizes` operands are non-negative.
 
     This interface is useful to factor out common behavior and provide support
     for carrying or injecting static behavior through the use of the static
diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
index 901784d7e369c8e..b66b413fbf89872 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -2852,17 +2852,6 @@ static LogicalResult produceSubViewErrorMsg(SliceVerificationResult result,
 }
 
 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/Interfaces/ViewLikeInterface.cpp b/mlir/lib/Interfaces/ViewLikeInterface.cpp
index 9247f571d9ab2c3..7c5905010eb4137 100644
--- a/mlir/lib/Interfaces/ViewLikeInterface.cpp
+++ b/mlir/lib/Interfaces/ViewLikeInterface.cpp
@@ -66,6 +66,17 @@ mlir::detail::verifyOffsetSizeAndStrideOp(OffsetSizeAndStrideOpInterface op) {
   if (failed(verifyListOfOperandsOrIntegers(
           op, "stride", maxRanks[2], op.getStaticStrides(), op.getStrides())))
     return failure();
+
+  for (int64_t offset : op.getStaticOffsets()) {
+    if (offset < 0 && !ShapedType::isDynamic(offset))
+      return op->emitError("expected offsets to be non-negative, but got ")
+             << offset;
+  }
+  for (int64_t size : op.getStaticSizes()) {
+    if (size < 0 && !ShapedType::isDynamic(size))
+      return op->emitError("expected sizes to be non-negative, but got ")
+             << size;
+  }
   return success();
 }
 
diff --git a/mlir/test/Dialect/MemRef/invalid.mlir b/mlir/test/Dialect/MemRef/invalid.mlir
index 38c0bcc3f2491c2..55b759cbb3ce7cd 100644
--- a/mlir/test/Dialect/MemRef/invalid.mlir
+++ b/mlir/test/Dialect/MemRef/invalid.mlir
@@ -612,7 +612,7 @@ 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}}
+  // expected-error at +1 {{expected 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>>
 }
@@ -620,7 +620,7 @@ func.func @invalid_subview(%input: memref<4x1024xf32>) -> memref<2x256xf32, stri
 // -----
 
 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}}
+  // expected-error at +1 {{expected 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>>
 }



More information about the cfe-commits mailing list