[mlir] Fix TileUsingForOp attr-dict printing/parsing, cleanup assembly format
#72745
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…bly 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]`
|
@llvm/pr-subscribers-mlir-linalg Author: Felix Schneider (ubfx) Changes
Full diff: https://github.com/llvm/llvm-project/pull/72745.diff 2 Files Affected:
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
}
|
|
@llvm/pr-subscribers-mlir Author: Felix Schneider (ubfx) Changes
Full diff: https://github.com/llvm/llvm-project/pull/72745.diff 2 Files Affected:
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
}
|
ftynse
requested changes
Nov 20, 2023
ftynse
reviewed
Nov 20, 2023
Done, I wasn't sure about this one because it seems like the majority of the other transform Ops tend towards special printing of the inherent Attributes. Is there a rule for when to prefer one or the other? |
Yes: please consider parsing inherent attribute as part of the discardable attributes dictionary deprecated. |
ftynse
approved these changes
Nov 22, 2023
OK, but in this case we can keep the old assembly format or should we add the special printing/parsing back? |
|
I suppose we need new assembly, but it should be fine landing this first (this is a bugfix) and working on a better assembly format that is consistent across all operations in the dialect in a separate stage. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TileUsingForOphas an optional Attributeinterchangewhich was given in curly braces like this:{interchange = [...]}. The way this was parsed meant that no normalattr-dictcould be attached to the Op.This patch changes the assembly format of the op to represent the
interchangeAttribute 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]