[Mlir-commits] [mlir] Reland "[mlir] Fix TileUsingForOp attr-dict printing/parsing" (PR #73261)
Felix Schneider
llvmlistbot at llvm.org
Thu Nov 23 11:48:31 PST 2023
https://github.com/ubfx created https://github.com/llvm/llvm-project/pull/73261
Reland https://github.com/llvm/llvm-project/pull/72745
fixed the failing test
Original message:
[mlir] Fix TileUsingForOp attr-dict printing/parsing (https://github.com/llvm/llvm-project/pull/72745)
`TileUsingForOp` has an optional Attribute `interchange` which was given
in curly braces like this: `{interchange = [...]}`. The way this was
parsed meant that no normal `attr-dict` could be attached to the Op.
This patch adds printing / parsing of an `attr-dict` to the Op and treats
the `interchange` Attribute as part of that dictionary for now.
>From d6c0286282d048b8b676a7ccf4e07b431c218e07 Mon Sep 17 00:00:00 2001
From: Felix Schneider <fx.schn at gmail.com>
Date: Fri, 17 Nov 2023 11:06:48 +0000
Subject: [PATCH 1/3] [mlir] Fix `TileUsingForOp` attr-dict printing/parsing,
cleanup assembly format
`TileUsingForOp` has an optional Attribute `interchange` which was
given in curly braces like this: `{ interchange = [...] }`.
The way this was parsed meant that no normal `attr-dict` could be
attached to the Op.
This patch changes the assembly format of the op to represent the
`interchange` Attribute more like other array Attributes in the
transform Ops and adds printing/parsing of an optional attr-dict.
`transform.structured.tile_using_for %0 [5, 6] interchange [1, 0]`
---
.../TransformOps/LinalgTransformOps.cpp | 24 +++++++++----------
.../Dialect/Linalg/transform-patterns.mlir | 6 ++---
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
index de4965f937162ea..943621dcfeb1739 100644
--- a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
+++ b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
@@ -2652,26 +2652,20 @@ SmallVector<OpFoldResult> transform::TileUsingForOp::getMixedSizes() {
// `array` prefix to be consistent in the IR with `parseDynamicIndexList`.
ParseResult parseOptionalInterchange(OpAsmParser &parser,
OperationState &result) {
- if (succeeded(parser.parseOptionalLBrace())) {
- if (failed(parser.parseKeyword("interchange")))
- return parser.emitError(parser.getNameLoc()) << "expect `interchange`";
- if (failed(parser.parseEqual()))
- return parser.emitError(parser.getNameLoc()) << "expect `=`";
- result.addAttribute("interchange",
- DenseI64ArrayAttr::parse(parser, Type{}));
- if (failed(parser.parseRBrace()))
- return parser.emitError(parser.getNameLoc()) << "expect `}`";
- }
+ if (succeeded(parser.parseOptionalKeyword("interchange")))
+ result.addAttribute(
+ transform::TileUsingForOp::getInterchangeAttrName(result.name),
+ DenseI64ArrayAttr::parse(parser, Type{}));
return success();
}
void printOptionalInterchange(OpAsmPrinter &p,
ArrayRef<int64_t> interchangeVals) {
if (!interchangeVals.empty()) {
- p << " {interchange = [";
+ p << " interchange [";
llvm::interleaveComma(interchangeVals, p,
[&](int64_t integer) { p << integer; });
- p << "]}";
+ p << "]";
}
}
@@ -2687,6 +2681,7 @@ ParseResult transform::TileUsingForOp::parse(OpAsmParser &parser,
if (parser.parseOperand(target) || parser.getCurrentLocation(&operandLoc) ||
parseDynamicIndexList(parser, dynamicSizes, staticSizes, scalableVals) ||
parseOptionalInterchange(parser, result) ||
+ parser.parseOptionalAttrDict(result.attributes) ||
parser.parseColonType(functionalType))
return ParseResult::failure();
@@ -2721,6 +2716,11 @@ void TileUsingForOp::print(OpAsmPrinter &p) {
/*valueTypes=*/{}, getScalableSizesAttr(),
OpAsmParser::Delimiter::Square);
printOptionalInterchange(p, getInterchange());
+ p.printOptionalAttrDict(
+ (*this)->getAttrs(),
+ /*elidedAttrs=*/{getInterchangeAttrName(getOperation()->getName()),
+ getScalableSizesAttrName(getOperation()->getName()),
+ getStaticSizesAttrName(getOperation()->getName())});
p << " : ";
p.printFunctionalType(getOperands().getTypes(), getResults().getTypes());
}
diff --git a/mlir/test/Dialect/Linalg/transform-patterns.mlir b/mlir/test/Dialect/Linalg/transform-patterns.mlir
index 77ce4d0b211f0d7..1c11842fc9b8c30 100644
--- a/mlir/test/Dialect/Linalg/transform-patterns.mlir
+++ b/mlir/test/Dialect/Linalg/transform-patterns.mlir
@@ -170,7 +170,7 @@ func.func @matvec_perm(%A: memref<?x?xf32, strided<[?, 1], offset: ?>>,
module attributes {transform.with_named_sequence} {
transform.named_sequence @__transform_main(%arg1: !transform.any_op {transform.readonly}) {
%0 = transform.structured.match ops{["linalg.matvec"]} in %arg1 : (!transform.any_op) -> !transform.any_op
- %1, %loops:2 = transform.structured.tile_using_for %0 [5, 6] {interchange = [1, 0]} : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op)
+ %1, %loops:2 = transform.structured.tile_using_for %0 [5, 6] interchange [1, 0] : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op)
transform.yield
}
}
@@ -199,8 +199,8 @@ func.func @matmul_perm(%A: memref<?x?xf32, strided<[?, 1], offset: ?>>,
module attributes {transform.with_named_sequence} {
transform.named_sequence @__transform_main(%arg1: !transform.any_op {transform.readonly}) {
%0 = transform.structured.match ops{["linalg.matmul"]} in %arg1 : (!transform.any_op) -> !transform.any_op
- %1, %loops:3 = transform.structured.tile_using_for %0 [2000, 3000, 4000] {interchange = [1, 2, 0]} : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op, !transform.any_op)
- %2, %loops_2:3 = transform.structured.tile_using_for %1 [200, 300, 400] {interchange = [1, 0, 2]} : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op, !transform.any_op)
+ %1, %loops:3 = transform.structured.tile_using_for %0 [2000, 3000, 4000] interchange [1, 2, 0] : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op, !transform.any_op)
+ %2, %loops_2:3 = transform.structured.tile_using_for %1 [200, 300, 400] interchange [1, 0, 2] : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op, !transform.any_op)
%3, %loops_3:3 = transform.structured.tile_using_for %2 [20, 30, 40] : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op, !transform.any_op)
transform.yield
}
>From f552e2050a5eeba37146000996f194362b50757e Mon Sep 17 00:00:00 2001
From: Felix Schneider <fx.schn at gmail.com>
Date: Tue, 21 Nov 2023 21:10:54 +0100
Subject: [PATCH 2/3] Incorporate review comments
---
.../Linalg/TransformOps/LinalgTransformOps.td | 2 +-
.../TransformOps/LinalgTransformOps.cpp | 31 +++----------------
mlir/test/Dialect/Linalg/transform-ops.mlir | 8 +++++
.../Dialect/Linalg/transform-patterns.mlir | 6 ++--
4 files changed, 16 insertions(+), 31 deletions(-)
diff --git a/mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td b/mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
index f1c3d717f1fa951..c8f0806e27a6264 100644
--- a/mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
+++ b/mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
@@ -1819,7 +1819,7 @@ def TileUsingForOp : Op<Transform_Dialect, "structured.tile_using_for",
let arguments = (ins TransformHandleTypeInterface:$target,
Variadic<TransformParamTypeOrAnyHandle>:$dynamic_sizes,
DefaultValuedOptionalAttr<DenseI64ArrayAttr, "{}">:$static_sizes,
- DefaultValuedOptionalAttr<DenseI64ArrayAttr, "{}">:$interchange,
+ DefaultValuedOptionalAttr<I64ArrayAttr, "{}">:$interchange,
DefaultValuedOptionalAttr<DenseBoolArrayAttr, "{}">:$scalable_sizes);
let results = (outs TransformHandleTypeInterface:$tiled_linalg_op,
Variadic<TransformHandleTypeInterface>:$loops);
diff --git a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
index 943621dcfeb1739..73de3f22d896f0a 100644
--- a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
+++ b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
@@ -2477,7 +2477,7 @@ void transform::TileUsingForOp::build(
/*target=*/target,
/*dynamic_sizes=*/dynamicTileSizes,
/*static_sizes=*/staticTileSizesAttr,
- /*interchange=*/builder.getDenseI64ArrayAttr(interchange),
+ /*interchange=*/builder.getI64ArrayAttr(interchange),
/*scalable_sizes=*/expandedScalableSizes);
}
@@ -2611,7 +2611,8 @@ transform::TileUsingForOp::apply(transform::TransformRewriter &rewriter,
});
}
- tilingOptions.setInterchange(getInterchange());
+ tilingOptions.setInterchange(
+ extractFromIntegerArrayAttr<int64_t>(getInterchange()));
FailureOr<scf::SCFTilingResult> maybeTilingResult =
tileUsingSCFForOp(rewriter, tilingInterface, tilingOptions);
if (failed(maybeTilingResult))
@@ -2648,27 +2649,6 @@ SmallVector<OpFoldResult> transform::TileUsingForOp::getMixedSizes() {
return results;
}
-// We want to parse `DenseI64ArrayAttr` using the short form without the
-// `array` prefix to be consistent in the IR with `parseDynamicIndexList`.
-ParseResult parseOptionalInterchange(OpAsmParser &parser,
- OperationState &result) {
- if (succeeded(parser.parseOptionalKeyword("interchange")))
- result.addAttribute(
- transform::TileUsingForOp::getInterchangeAttrName(result.name),
- DenseI64ArrayAttr::parse(parser, Type{}));
- return success();
-}
-
-void printOptionalInterchange(OpAsmPrinter &p,
- ArrayRef<int64_t> interchangeVals) {
- if (!interchangeVals.empty()) {
- p << " interchange [";
- llvm::interleaveComma(interchangeVals, p,
- [&](int64_t integer) { p << integer; });
- p << "]";
- }
-}
-
ParseResult transform::TileUsingForOp::parse(OpAsmParser &parser,
OperationState &result) {
OpAsmParser::UnresolvedOperand target;
@@ -2680,7 +2660,6 @@ ParseResult transform::TileUsingForOp::parse(OpAsmParser &parser,
if (parser.parseOperand(target) || parser.getCurrentLocation(&operandLoc) ||
parseDynamicIndexList(parser, dynamicSizes, staticSizes, scalableVals) ||
- parseOptionalInterchange(parser, result) ||
parser.parseOptionalAttrDict(result.attributes) ||
parser.parseColonType(functionalType))
return ParseResult::failure();
@@ -2715,11 +2694,9 @@ void TileUsingForOp::print(OpAsmPrinter &p) {
printDynamicIndexList(p, getOperation(), getDynamicSizes(), getStaticSizes(),
/*valueTypes=*/{}, getScalableSizesAttr(),
OpAsmParser::Delimiter::Square);
- printOptionalInterchange(p, getInterchange());
p.printOptionalAttrDict(
(*this)->getAttrs(),
- /*elidedAttrs=*/{getInterchangeAttrName(getOperation()->getName()),
- getScalableSizesAttrName(getOperation()->getName()),
+ /*elidedAttrs=*/{getScalableSizesAttrName(getOperation()->getName()),
getStaticSizesAttrName(getOperation()->getName())});
p << " : ";
p.printFunctionalType(getOperands().getTypes(), getResults().getTypes());
diff --git a/mlir/test/Dialect/Linalg/transform-ops.mlir b/mlir/test/Dialect/Linalg/transform-ops.mlir
index e9f044be5b4ed22..4d7c514dcca62d5 100644
--- a/mlir/test/Dialect/Linalg/transform-ops.mlir
+++ b/mlir/test/Dialect/Linalg/transform-ops.mlir
@@ -6,6 +6,14 @@ transform.sequence failures(propagate) {
%0, %1:2 = transform.structured.tile_using_for %arg0 [2, 0, 3] : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op)
}
+// check that the Attributes of `tile_using_for` are preserved through printing
+// and parsing.
+transform.sequence failures(propagate) {
+^bb1(%arg0: !transform.any_op):
+ // CHECK %{{.*}}, %{{.*}}:2 = transform.structured.tile %arg0 [2, 0, 3] {interchange = [2, 1], test_attr1 = 1 : i64, test_attr2}
+ %0, %1:2 = transform.structured.tile_using_for %arg0 [2, 0, 3] {test_attr1 = 1 : i64, interchange = [2, 1], test_attr2}: (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op)
+}
+
transform.sequence failures(propagate) {
^bb1(%arg0: !transform.any_op):
%0:2 = transform.structured.split %arg0 after 42 { dimension = 0 } : !transform.any_op
diff --git a/mlir/test/Dialect/Linalg/transform-patterns.mlir b/mlir/test/Dialect/Linalg/transform-patterns.mlir
index 1c11842fc9b8c30..77ce4d0b211f0d7 100644
--- a/mlir/test/Dialect/Linalg/transform-patterns.mlir
+++ b/mlir/test/Dialect/Linalg/transform-patterns.mlir
@@ -170,7 +170,7 @@ func.func @matvec_perm(%A: memref<?x?xf32, strided<[?, 1], offset: ?>>,
module attributes {transform.with_named_sequence} {
transform.named_sequence @__transform_main(%arg1: !transform.any_op {transform.readonly}) {
%0 = transform.structured.match ops{["linalg.matvec"]} in %arg1 : (!transform.any_op) -> !transform.any_op
- %1, %loops:2 = transform.structured.tile_using_for %0 [5, 6] interchange [1, 0] : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op)
+ %1, %loops:2 = transform.structured.tile_using_for %0 [5, 6] {interchange = [1, 0]} : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op)
transform.yield
}
}
@@ -199,8 +199,8 @@ func.func @matmul_perm(%A: memref<?x?xf32, strided<[?, 1], offset: ?>>,
module attributes {transform.with_named_sequence} {
transform.named_sequence @__transform_main(%arg1: !transform.any_op {transform.readonly}) {
%0 = transform.structured.match ops{["linalg.matmul"]} in %arg1 : (!transform.any_op) -> !transform.any_op
- %1, %loops:3 = transform.structured.tile_using_for %0 [2000, 3000, 4000] interchange [1, 2, 0] : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op, !transform.any_op)
- %2, %loops_2:3 = transform.structured.tile_using_for %1 [200, 300, 400] interchange [1, 0, 2] : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op, !transform.any_op)
+ %1, %loops:3 = transform.structured.tile_using_for %0 [2000, 3000, 4000] {interchange = [1, 2, 0]} : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op, !transform.any_op)
+ %2, %loops_2:3 = transform.structured.tile_using_for %1 [200, 300, 400] {interchange = [1, 0, 2]} : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op, !transform.any_op)
%3, %loops_3:3 = transform.structured.tile_using_for %2 [20, 30, 40] : (!transform.any_op) -> (!transform.any_op, !transform.any_op, !transform.any_op, !transform.any_op)
transform.yield
}
>From 3b1b0c8061f81ea9bdfecf8bc8f17449833d6dcc Mon Sep 17 00:00:00 2001
From: Felix Schneider <fx.schn at gmail.com>
Date: Thu, 23 Nov 2023 20:35:50 +0100
Subject: [PATCH 3/3] Fix python test
---
mlir/test/python/dialects/transform_structured_ext.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mlir/test/python/dialects/transform_structured_ext.py b/mlir/test/python/dialects/transform_structured_ext.py
index c9b7802e1cc4532..1c7552e2bd51a80 100644
--- a/mlir/test/python/dialects/transform_structured_ext.py
+++ b/mlir/test/python/dialects/transform_structured_ext.py
@@ -344,7 +344,7 @@ def testSplit(target):
@run
@create_sequence
def testTileCompact(target):
- structured.TileUsingForOp(target, sizes=[4, 8], interchange=[0, 1])
+ structured.TileUsingForOp(target, sizes=[4, 8], interchange=Attribute.parse("[0, 1]"))
# CHECK-LABEL: TEST: testTileCompact
# CHECK: transform.sequence
# CHECK: %{{.+}}, %{{.+}}:2 = transform.structured.tile_using_for %{{.*}}[4, 8]
@@ -355,7 +355,7 @@ def testTileCompact(target):
@create_sequence
def testTileAttributes(target):
attr = DenseI64ArrayAttr.get([4, 8])
- ichange = DenseI64ArrayAttr.get([0, 1])
+ ichange = Attribute.parse("[0, 1]")
structured.TileUsingForOp(target, sizes=attr, interchange=ichange)
# CHECK-LABEL: TEST: testTileAttributes
# CHECK: transform.sequence
More information about the Mlir-commits
mailing list