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 1/8] [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 2/8] 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 3/8] 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 4/8] 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 5/8] 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 6/8] 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 7/8] 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 8/8] 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"); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits