Enable printing newlines and indents in attribute and type printers #87948
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Jacenty Andruszkiewicz (Jacenty-And-Intel) ChangesThis commit moves the code responsible for adding newlines and tracking indent, so that it can be used not only for operation printers, but also for attribute and type printers. Full diff: https://github.com/llvm/llvm-project/pull/87948.diff 9 Files Affected:
diff --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index 5333d7446df5cab..64a9184c5c6720b 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -116,6 +116,16 @@ class AsmPrinter {
/// Return the raw output stream used by this printer.
virtual raw_ostream &getStream() const;
+ /// Print a newline and indent the printer to the start of the current
+ /// operation.
+ virtual void printNewline();
+
+ /// Increase indentation.
+ virtual void increaseIndent();
+
+ /// Decrease indentation.
+ virtual void decreaseIndent();
+
/// Print the given floating point value in a stabilized form that can be
/// roundtripped through the IR. This is the companion to the 'parseFloat'
/// hook on the AsmParser.
@@ -417,16 +427,6 @@ class OpAsmPrinter : public AsmPrinter {
/// Print a loc(...) specifier if printing debug info is enabled.
virtual void printOptionalLocationSpecifier(Location loc) = 0;
- /// Print a newline and indent the printer to the start of the current
- /// operation.
- virtual void printNewline() = 0;
-
- /// Increase indentation.
- virtual void increaseIndent() = 0;
-
- /// Decrease indentation.
- virtual void decreaseIndent() = 0;
-
/// Print a block argument in the usual format of:
/// %ssaName : type {attr1=42} loc("here")
/// where location printing is controlled by the standard internal option.
diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index 456cf6a2c27783e..fc5c142fc433bb8 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -376,6 +376,19 @@ class AsmPrinter::Impl {
/// Returns the output stream of the printer.
raw_ostream &getStream() { return os; }
+ /// Print a newline and indent the printer to the start of the current
+ /// operation.
+ void printNewline() {
+ os << newLine;
+ os.indent(currentIndent);
+ }
+
+ /// Increase indentation.
+ void increaseIndent() { currentIndent += indentWidth; }
+
+ /// Decrease indentation.
+ void decreaseIndent() { currentIndent -= indentWidth; }
+
template <typename Container, typename UnaryFunctor>
inline void interleaveComma(const Container &c, UnaryFunctor eachFn) const {
llvm::interleaveComma(c, os, eachFn);
@@ -490,6 +503,12 @@ class AsmPrinter::Impl {
/// A tracker for the number of new lines emitted during printing.
NewLineCounter newLine;
+
+ /// The number of spaces used for indenting nested operations.
+ const static unsigned indentWidth = 2;
+
+ /// This is the current indentation level for nested structures.
+ unsigned currentIndent = 0;
};
} // namespace mlir
@@ -942,6 +961,9 @@ class DummyAliasDialectAsmPrinter : public DialectAsmPrinter {
/// The following are hooks of `DialectAsmPrinter` that are not necessary for
/// determining potential aliases.
+ void printNewline() override {}
+ void increaseIndent() override {}
+ void decreaseIndent() override {}
void printFloat(const APFloat &) override {}
void printKeywordOrString(StringRef) override {}
void printString(StringRef) override {}
@@ -2708,6 +2730,13 @@ void AsmPrinter::Impl::printDialectAttribute(Attribute attr) {
{
llvm::raw_string_ostream attrNameStr(attrName);
Impl subPrinter(attrNameStr, state);
+
+ // The values of currentIndent and newLine are assigned to the created
+ // subprinter, so that the indent level and number of printed lines can be
+ // tracked.
+ subPrinter.currentIndent = currentIndent;
+ subPrinter.newLine = newLine;
+
DialectAsmPrinter printer(subPrinter);
dialect.printAttribute(attr, printer);
}
@@ -2722,6 +2751,13 @@ void AsmPrinter::Impl::printDialectType(Type type) {
{
llvm::raw_string_ostream typeNameStr(typeName);
Impl subPrinter(typeNameStr, state);
+
+ // The values of currentIndent and newLine are assigned to the created
+ // subprinter, so that the indent level and number of printed lines can be
+ // tracked.
+ subPrinter.currentIndent = currentIndent;
+ subPrinter.newLine = newLine;
+
DialectAsmPrinter printer(subPrinter);
dialect.printType(type, printer);
}
@@ -2762,6 +2798,21 @@ raw_ostream &AsmPrinter::getStream() const {
return impl->getStream();
}
+void AsmPrinter::printNewline() {
+ assert(impl && "expected AsmPrinter::printNewLine to be overriden");
+ impl->printNewline();
+}
+
+void AsmPrinter::increaseIndent() {
+ assert(impl && "expected AsmPrinter::increaseIndent to be overriden");
+ impl->increaseIndent();
+}
+
+void AsmPrinter::decreaseIndent() {
+ assert(impl && "expected AsmPrinter::decreaseIndent to be overriden");
+ impl->decreaseIndent();
+}
+
/// Print the given floating point value in a stablized form.
void AsmPrinter::printFloat(const APFloat &value) {
assert(impl && "expected AsmPrinter::printFloat to be overriden");
@@ -3087,19 +3138,6 @@ class OperationPrinter : public AsmPrinter::Impl, private OpAsmPrinter {
printTrailingLocation(loc);
}
- /// Print a newline and indent the printer to the start of the current
- /// operation.
- void printNewline() override {
- os << newLine;
- os.indent(currentIndent);
- }
-
- /// Increase indentation.
- void increaseIndent() override { currentIndent += indentWidth; }
-
- /// Decrease indentation.
- void decreaseIndent() override { currentIndent -= indentWidth; }
-
/// Print a block argument in the usual format of:
/// %ssaName : type {attr1=42} loc("here")
/// where location printing is controlled by the standard internal option.
@@ -3225,12 +3263,6 @@ class OperationPrinter : public AsmPrinter::Impl, private OpAsmPrinter {
// top-level we start with "builtin" as the default, so that the top-level
// `module` operation prints as-is.
SmallVector<StringRef> defaultDialectStack{"builtin"};
-
- /// The number of spaces used for indenting nested operations.
- const static unsigned indentWidth = 2;
-
- // This is the current indentation level for nested structures.
- unsigned currentIndent = 0;
};
} // namespace
diff --git a/mlir/test/lib/Dialect/Test/TestAttrDefs.td b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
index 40f035a3e3a4e5e..7d01979f45a055d 100644
--- a/mlir/test/lib/Dialect/Test/TestAttrDefs.td
+++ b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
@@ -340,4 +340,11 @@ def TestConditionalAliasAttr : Test_Attr<"TestConditionalAlias"> {
}];
}
+def TestAttrNewlineAndIndent : Test_Attr<"TestAttrNewlineAndIndent"> {
+ let mnemonic = "newline_and_indent";
+ let parameters = (ins "::mlir::Type":$indentType);
+ let hasCustomAssemblyFormat = 1;
+}
+
+
#endif // TEST_ATTRDEFS
diff --git a/mlir/test/lib/Dialect/Test/TestAttributes.cpp b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
index d41d495c38e5532..605fa861da823e5 100644
--- a/mlir/test/lib/Dialect/Test/TestAttributes.cpp
+++ b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
@@ -239,6 +239,35 @@ static void printConditionalAlias(AsmPrinter &p, StringAttr value) {
p.printKeywordOrString(value);
}
+//===----------------------------------------------------------------------===//
+// TestAttrNewlineAndIndent
+//===----------------------------------------------------------------------===//
+
+Attribute TestAttrNewlineAndIndentAttr::parse(::mlir::AsmParser &parser,
+ ::mlir::Type type) {
+ Type indentType;
+ if (parser.parseLess()) {
+ return {};
+ }
+ if (parser.parseType(indentType)) {
+ return {};
+ }
+ if (parser.parseGreater()) {
+ return {};
+ }
+ return get(parser.getContext(), indentType);
+}
+
+void TestAttrNewlineAndIndentAttr::print(::mlir::AsmPrinter &printer) const {
+ printer << "<";
+ printer.increaseIndent();
+ printer.printNewline();
+ printer << getIndentType();
+ printer.decreaseIndent();
+ printer.printNewline();
+ printer << ">";
+}
+
//===----------------------------------------------------------------------===//
// Tablegen Generated Definitions
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/lib/Dialect/Test/TestTypeDefs.td b/mlir/test/lib/Dialect/Test/TestTypeDefs.td
index 492642b711e09e8..842af153e98d338 100644
--- a/mlir/test/lib/Dialect/Test/TestTypeDefs.td
+++ b/mlir/test/lib/Dialect/Test/TestTypeDefs.td
@@ -392,4 +392,9 @@ def TestRecursiveAlias
}];
}
+def TestTypeNewlineAndIndent : Test_Type<"TestTypeNewlineAndIndent"> {
+ let mnemonic = "newline_and_indent";
+ let hasCustomAssemblyFormat = 1;
+}
+
#endif // TEST_TYPEDEFS
diff --git a/mlir/test/lib/Dialect/Test/TestTypes.cpp b/mlir/test/lib/Dialect/Test/TestTypes.cpp
index 7a195eb25a3ba1a..21bb2e3efcb805a 100644
--- a/mlir/test/lib/Dialect/Test/TestTypes.cpp
+++ b/mlir/test/lib/Dialect/Test/TestTypes.cpp
@@ -530,3 +530,30 @@ void TestRecursiveAliasType::print(AsmPrinter &printer) const {
}
printer << ">";
}
+
+//===----------------------------------------------------------------------===//
+// TestTypeNewlineAndIndent
+//===----------------------------------------------------------------------===//
+
+Type TestTypeNewlineAndIndentType::parse(::mlir::AsmParser &parser) {
+ if (parser.parseLess()) {
+ return {};
+ }
+ if (parser.parseKeyword("indented_content")) {
+ return {};
+ }
+ if (parser.parseGreater()) {
+ return {};
+ }
+ return get(parser.getContext());
+}
+
+void TestTypeNewlineAndIndentType::print(::mlir::AsmPrinter &printer) const {
+ printer << "<";
+ printer.increaseIndent();
+ printer.printNewline();
+ printer << "indented_content";
+ printer.decreaseIndent();
+ printer.printNewline();
+ printer << ">";
+}
\ No newline at end of file
diff --git a/mlir/test/mlir-tblgen/testdialect-attrdefs.mlir b/mlir/test/mlir-tblgen/testdialect-attrdefs.mlir
index ee92ea06a208c47..1eba4f595694a10 100644
--- a/mlir/test/mlir-tblgen/testdialect-attrdefs.mlir
+++ b/mlir/test/mlir-tblgen/testdialect-attrdefs.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s | mlir-opt -verify-diagnostics | FileCheck %s
+// RUN: mlir-opt %s | mlir-opt -verify-diagnostics | FileCheck %s --strict-whitespace
// CHECK-LABEL: func private @compoundA()
// CHECK-SAME: #test.cmpnd_a<1, !test.smpla, [5, 6]>
@@ -19,3 +19,17 @@ func.func private @qualifiedAttr() attributes {foo = #test.cmpnd_nested_outer_qu
func.func private @overriddenAttr() attributes {
foo = #test.override_builder<5>
}
+
+// CHECK-LABEL: @newlineAndIndent
+// CHECK-SAME: indent = #test.newline_and_indent<
+// CHECK-NEXT: {{^ }}!test.newline_and_indent<
+// CHECK-NEXT: {{^ }}indented_content
+// CHECK-NEXT: {{^ }}>
+// CHECK-NEXT: {{^ }}>
+func.func private @newlineAndIndent() attributes {
+ indent = #test.newline_and_indent<
+ !test.newline_and_indent<
+ indented_content
+ >
+ >
+}
diff --git a/mlir/test/mlir-tblgen/testdialect-typedefs.mlir b/mlir/test/mlir-tblgen/testdialect-typedefs.mlir
index 18175edc81cf08b..c00d368fdab8ba8 100644
--- a/mlir/test/mlir-tblgen/testdialect-typedefs.mlir
+++ b/mlir/test/mlir-tblgen/testdialect-typedefs.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s | mlir-opt -verify-diagnostics | FileCheck %s
+// RUN: mlir-opt %s | mlir-opt -verify-diagnostics | FileCheck %s --strict-whitespace
//////////////
// Tests the types in the 'Test' dialect, not the ones in 'typedefs.mlir'
@@ -42,3 +42,13 @@ func.func @testInt(%A : !test.int<s, 8>, %B : !test.int<unsigned, 2>, %C : !test
func.func @structTest (%A : !test.struct< {field1, !test.smpla}, {field2, !test.int<none, 3>} > ) {
return
}
+
+// CHECK-LABEL: @newlineAndIndent
+// CHECK-SAME: !test.newline_and_indent<
+// CHECK-NEXT: {{^ }}indented_content
+// CHECK-NEXT: {{^ }}>
+func.func @newlineAndIndent(%A : !test.newline_and_indent<
+ indented_content
+>) {
+ return
+}
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
index 6098808c646f765..2706e48945d587a 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
@@ -903,9 +903,7 @@ void DefFormat::genOptionalGroupPrinter(OptionalElement *el, FmtContext &ctx,
void DefFormat::genWhitespacePrinter(WhitespaceElement *el, FmtContext &ctx,
MethodBody &os) {
if (el->getValue() == "\\n") {
- // FIXME: The newline should be `printer.printNewLine()`, i.e., handled by
- // the printer.
- os << tgfmt("$_printer << '\\n';\n", &ctx);
+ os << tgfmt("$_printer.printNewline();\n", &ctx);
} else if (!el->getValue().empty()) {
os << tgfmt("$_printer << \"$0\";\n", &ctx, el->getValue());
} else {
|
|
Your PR description says "what" the commit is doing, but is not providing any motivation or context: can you elaborate on this? (see guidelines: https://mlir.llvm.org/getting_started/Contributing/#commit-messages ) Some of my questions will be about the composability: the operation is in control of the format in general, and having types randomly introducing new lines may not be actually composable. |
Jacenty-And-Intel
force-pushed
the
AsmPrinter_formatting
branch
from
6804e64 to
37c1bd0
Compare
|
Introducing new lines by types or attributes could be used in justified cases in custom printers where it would improve the readability of the IR. In some cases where there are multiple levels of nesting and lists of attributes, it could be difficult to get useful information while debugging. The indent in nested printers is tracked and would be preserved when the new line is added. Example without formatting: Same example with formatting: |
|
How does it play in practice though? Attributes and types aren't printed in isolation in general (other than aliases). What are examples of this when attached to an operation? |
|
Here is an example when attribute with nesting is attached to an operation: |
|
Right: this is what I meant to point at in terms of composability of the printing: the readability of an op with multiple attribute seems to be quite context dependent. Is this printing always desirable in the IR? While focusing on this one attribute seems better, the overall IR flow can become unbearable. |
This is more of an enabler than a prescriptive change and it does not affect any existing IR formatting. |
|
Seems the build has failed due to OOM |
|
@joker-eph Were all of your concerns been addressed? If so, could you please re-review? |
|
To address my concerns, I think you'd need to have the control be contextual to the operation. That is by default printing the attribute would not have any line break, but the operation assembly format could opt-in into this. That can be achieved by having a flag on the AsmPrinter for example to allow the line break and the operation setting/unsetting the flag on specific attributes. |
@joker-eph I read the attribute is printed either with or without newlines depending on the parent operation? In the proposal it's the attribute printer's implementation choice and it is transparent to the parent operation, even if the operation uses newlines semantically the parsing composes (operation parser won't see attribute line breaks). If we need a context specific escape hatch, perhaps an option to disable pretty printers on the operation level would be the way to go? |
Right, this is the part I'm not convinced about :) I expressed it above already I believe: #87948 (comment)
Are there precedent for doing this? I don't remember seeing this, and don't even remember we added support for this actually.
I believe the printer itself could elide calls to |
Well, for the use cases I can think of, yes it would be always desired to have the newlines if attribute printer chooses so.
Not sure it was ever used semantically for ops. I'm guessing it's ultimately decision of the pretty printer. Not necessarily to my taste, but semantic NLs could be helpful at least for prototyping. NLs in attribute syntax do not seem to impair readability of the ops in our dialect, as the scope of attribute is very clear.
What I'm worried about is that the such control mechanism may open a can of worms. Not sure how we inform the printer also not sure whether should that be per op or perhaps a knob controlled externally like command line? At least in our use case, the elaborate attributes are specific to a handful of ops and we have not encountered any issues with unconditional newlines in these. |
|
Just confirming that I'm reading this right: it enables attribute printers to be written to produce newlines/nesting, but that will only be done if an attribute opts in (and no existing attribute formats are changed)? |
That's right! |
Jacenty-And-Intel
force-pushed
the
AsmPrinter_formatting
branch
from
37c1bd0 to
fa8d55f
Compare
Jacenty-And-Intel
force-pushed
the
AsmPrinter_formatting
branch
from
fa8d55f to
d4e2c63
Compare
Jacenty-And-Intel
force-pushed
the
AsmPrinter_formatting
branch
2 times, most recently
from
d96da4e to
c35b190
Compare
Jacenty-And-Intel
force-pushed
the
AsmPrinter_formatting
branch
from
c35b190 to
3c01f4f
Compare
This commit moves the code responsible for adding newlines and tracking indent, so that it can be used not only for operation printers, but also for attribute and type printers. It could be useful for nested attributes, where proper formatting with newlines and indents would benefit the readability of the IR. Currently, everything is printed on one line, which makes it difficult to read if the attribute is more verbose and there are multiple levels of nesting.
Jacenty-And-Intel
force-pushed
the
AsmPrinter_formatting
branch
from
3c01f4f to
fafd534
Compare
|
@joker-eph Could you please re-review? Are there any more concerns left that should be addressed? |
|
This enables querying indentation. Attributes can already print multiline and make their parsers handle it, not that it documented as supported nor widely used. I think my main concern would be that such multiline attributes can obscure program structure (e.g., one goes from a form where most operations are single line, to one where it is variable, and knowing if a line is part of attribute or operation may require going 100 lines up or down to find start/end). And in which case emitting as aliases always is much better, and if emitted always as an alias, one knows exactly the indent already and this is not needed (e.g., dialect can check size of output, if large use alias, else no alias, and based on same size attribute can print indented or not). This would enable making it prettier for the non-alias case. But by making it an explicit option, this does move from possible to supported use. I do want a multiline string constant and it does help a lot for such cases where it isn't large. Does seem useful, but can be abused and make the IR less useful. Having it be context sensitive (e.g., inside alias or not, inside op that opt'd in or not) is not something we have today. I was thinking about the raw_indented_ostream and potential of swapping out indenting/newline output (e.g., if a regular raw_indented_ostream then its nested, if not its flattened) as a mechanism. But it would be the first time we have such a case ... well, no, that's not true: an operation can always decide to print and parse the attribute however it wishes, its only when it just delegates to attribute kind where that isn't true (that just happens to be the case >90% of the time!). And code duplication can be avoided c++ side. |
|
Limiting this feature to the case where the attribute is printing as an alias would be addressing my concerns actually. |
|
I understand the concern and how restricting the newline support to aliases encourages terse IR design. Perhaps we could allow the flexibility for attribute printers but instead of explicit restriction provide more guidance for IR design, but I would rather defer such decisions to specific dialects and I'm positive reviewers would spot this kind of issues if printers become too elaborate. |
|
Example of usage can be found in our public codebase. Newlines and nesting of attributes is used for operation descriptors. Here is a small snippet from IR with an operation that have implemented custom pretty printers and is using nested attributes: When printing descriptor as a regular attribute it looses readability and it's hard to analyze: The example above only presents small part of real operation. In real world scenario operations have hundreds of nested attributes what makes them completely unreadable when printed in one line. Here is an example of full operation with and without formatting of the nested attributes. |
|
@jpienaar, @joker-eph did you have a chance to examine the examples provided? It's a custom dialect where moving attributes out of line with aliases goes against readability as each of such operations is unique and should be considered stand-alone. |
|
I haven't had time to revisit this issue yet, I'm preparing for the conference and workshop next week, so will be busy until mid-November unfortunately. |
|
I hope the conference and workshop went well! Did you have a chance to revisit this issue and examine the provided examples? |
|
Apologies, we'll get back to you shortly and resolve next steps. Thanks for patience here. |
6 participants
This commit moves the code responsible for adding newlines and tracking indent, so that it can be used not only for operation printers, but also for attribute and type printers.
It could be useful for nested attributes, where proper formatting with newlines and indents would benefit the readability of the IR. Currently, everything is printed on one line, which makes it difficult to read if the attribute is more verbose and there are multiple levels of nesting.