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 <git...@huijzer.xyz> Date: Sat, 11 Nov 2023 20:43:31 +0100 Subject: [PATCH 01/12] [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@+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@+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@+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 <git...@huijzer.xyz> Date: Sun, 12 Nov 2023 09:10:00 +0100 Subject: [PATCH 02/12] 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@+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@+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 <git...@huijzer.xyz> Date: Sun, 12 Nov 2023 15:28:55 +0100 Subject: [PATCH 03/12] 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 <git...@huijzer.xyz> Date: Sun, 12 Nov 2023 17:48:11 +0100 Subject: [PATCH 04/12] 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 <git...@huijzer.xyz> Date: Sun, 12 Nov 2023 17:52:53 +0100 Subject: [PATCH 05/12] 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 <git...@huijzer.xyz> Date: Sun, 12 Nov 2023 17:58:21 +0100 Subject: [PATCH 06/12] 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 <git...@huijzer.xyz> Date: Sun, 12 Nov 2023 17:59:04 +0100 Subject: [PATCH 07/12] 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 <git...@huijzer.xyz> Date: Sun, 12 Nov 2023 18:49:45 +0100 Subject: [PATCH 08/12] 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 <git...@huijzer.xyz> Date: Mon, 13 Nov 2023 12:01:04 +0100 Subject: [PATCH 09/12] 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@+1 {{expected subview offsets to be non-negative, but got -1}} + // expected-error@+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@+1 {{expected subview sizes to be non-negative, but got -1}} + // expected-error@+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>> } >From 6a2f5b8808f5c3715400ed5e3393492eee996675 Mon Sep 17 00:00:00 2001 From: Rik Huijzer <git...@huijzer.xyz> Date: Mon, 13 Nov 2023 15:25:29 +0100 Subject: [PATCH 10/12] Remove outdated assertion --- mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp index b66b413fbf89872..5ee7f39a955c7c2 100644 --- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp +++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp @@ -2621,15 +2621,6 @@ 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); } >From d59fe3a566c33b66276cd0eb3b7c5b18246c8963 Mon Sep 17 00:00:00 2001 From: Rik Huijzer <git...@huijzer.xyz> Date: Mon, 13 Nov 2023 15:27:03 +0100 Subject: [PATCH 11/12] Put old comment back to avoid file touch --- mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp index 5ee7f39a955c7c2..aba46dc0a0d33ca 100644 --- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp +++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp @@ -2842,6 +2842,7 @@ static LogicalResult produceSubViewErrorMsg(SliceVerificationResult result, llvm_unreachable("unexpected subview verification result"); } +/// Verifier for SubViewOp. LogicalResult SubViewOp::verify() { MemRefType baseType = getSourceType(); MemRefType subViewType = getType(); >From 83934f02a7e149e18a67ad1fcace81a10730c60b Mon Sep 17 00:00:00 2001 From: Rik Huijzer <git...@huijzer.xyz> Date: Tue, 14 Nov 2023 20:42:40 +0100 Subject: [PATCH 12/12] Fix crash in canonicalizer --- mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp | 18 ++++++++++++++++-- mlir/test/Dialect/MemRef/canonicalize.mlir | 11 +++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp index aba46dc0a0d33ca..2b3db0a49d3208a 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/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]] _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits