[Mlir-commits] [mlir] 1949fe9 - [mlir] Verify non-negative `offset` and `size` (#72059)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Wed Nov 15 22:42:41 PST 2023
Author: Rik Huijzer
Date: 2023-11-16T07:42:37+01:00
New Revision: 1949fe90bfccaa13b4ecbce33de3123824b9a150
URL: https://github.com/llvm/llvm-project/commit/1949fe90bfccaa13b4ecbce33de3123824b9a150
DIFF: https://github.com/llvm/llvm-project/commit/1949fe90bfccaa13b4ecbce33de3123824b9a150.diff
LOG: [mlir] Verify non-negative `offset` and `size` (#72059)
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".
This patch fixes #71153 by not folding in aforementioned situation.
Also, this patch adds a basic offset and size check in the
`OffsetSizeAndStrideOpInterface` verifier.
Note: only `offset` and `size` are checked because `stride` is allowed
to be negative
(https://github.com/llvm/llvm-project/commit/54d81e49e3b72f6a305891fe169ecd7c6f559223).
Added:
Modified:
mlir/include/mlir/Interfaces/ViewLikeInterface.td
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
mlir/lib/Interfaces/ViewLikeInterface.cpp
mlir/test/Dialect/MemRef/canonicalize.mlir
mlir/test/Dialect/MemRef/invalid.mlir
Removed:
################################################################################
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 b901cd8497fe6d1..a2fc954ad07fae8 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -2621,6 +2621,17 @@ Type SubViewOp::inferResultType(MemRefType sourceMemRefType,
dispatchIndexOpFoldResults(offsets, dynamicOffsets, staticOffsets);
dispatchIndexOpFoldResults(sizes, dynamicSizes, staticSizes);
dispatchIndexOpFoldResults(strides, dynamicStrides, staticStrides);
+
+ // If one of the offsets or sizes is invalid, fail the canonicalization.
+ // These checks also occur in the verifier, but they are needed here
+ // because some dynamic dimensions may have been constant folded.
+ for (int64_t offset : staticOffsets)
+ if (offset < 0 && !ShapedType::isDynamic(offset))
+ return {};
+ for (int64_t size : staticSizes)
+ if (size < 0 && !ShapedType::isDynamic(size))
+ return {};
+
return SubViewOp::inferResultType(sourceMemRefType, staticOffsets,
staticSizes, staticStrides);
}
@@ -3094,8 +3105,11 @@ struct SubViewReturnTypeCanonicalizer {
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));
+ auto resTy = SubViewOp::inferResultType(op.getSourceType(), mixedOffsets,
+ mixedSizes, mixedStrides);
+ if (!resTy)
+ return {};
+ MemRefType nonReducedType = cast<MemRefType>(resTy);
// Directly return the non-rank reduced type if there are no dropped dims.
llvm::SmallBitVector droppedDims = op.getDroppedDims();
diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
index eec89be74b88c0f..e469815496e1832 100644
--- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
+++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
@@ -1261,7 +1261,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/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/canonicalize.mlir b/mlir/test/Dialect/MemRef/canonicalize.mlir
index c9f874e4cf3051e..a1f8673638ff813 100644
--- a/mlir/test/Dialect/MemRef/canonicalize.mlir
+++ b/mlir/test/Dialect/MemRef/canonicalize.mlir
@@ -180,6 +180,17 @@ func.func @dim_of_sized_view(%arg : memref<?xi8>, %size: index) -> index {
// -----
+// CHECK-LABEL: func @no_fold_subview_negative_size
+// CHECK: %[[SUBVIEW:.+]] = memref.subview
+// CHECK: return %[[SUBVIEW]]
+func.func @no_fold_subview_negative_size(%input: memref<4x1024xf32>) -> memref<?x256xf32, strided<[1024, 1], offset: 2304>> {
+ %cst = arith.constant -13 : index
+ %0 = memref.subview %input[2, 256] [%cst, 256] [1, 1] : memref<4x1024xf32> to memref<?x256xf32, strided<[1024, 1], offset: 2304>>
+ return %0 : memref<?x256xf32, strided<[1024, 1], offset: 2304>>
+}
+
+// -----
+
// CHECK-LABEL: func @no_fold_of_store
// CHECK: %[[cst:.+]] = memref.cast %arg
// CHECK: memref.store %[[cst]]
diff --git a/mlir/test/Dialect/MemRef/invalid.mlir b/mlir/test/Dialect/MemRef/invalid.mlir
index cb5977e302a993f..55b759cbb3ce7cd 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 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 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}}
More information about the Mlir-commits
mailing list