[Mlir-commits] [mlir] [mlir] [attribute] Reproduce inconsistent attribute parser and printer (PR #133872)
Jueon Park
llvmlistbot at llvm.org
Thu Oct 9 00:58:34 PDT 2025
https://github.com/JueonPark updated https://github.com/llvm/llvm-project/pull/133872
>From 6782561ceab3c3a7f2b6c439da5f2cd0632453ed Mon Sep 17 00:00:00 2001
From: Jueon Park <jueonpark at rebellions.ai>
Date: Tue, 1 Apr 2025 15:26:00 +0900
Subject: [PATCH 1/3] update(attribute,test): add nested attribute for test
---
mlir/test/lib/Dialect/Test/TestAttrDefs.td | 30 ++++++++++++++++++++++
mlir/test/mlir-tblgen/op-format.mlir | 21 +++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/mlir/test/lib/Dialect/Test/TestAttrDefs.td b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
index fc2d77af29f12..6282154b9b912 100644
--- a/mlir/test/lib/Dialect/Test/TestAttrDefs.td
+++ b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
@@ -58,6 +58,36 @@ def CompoundAttrNested : Test_Attr<"CompoundAttrNested"> {
let assemblyFormat = "`<` `nested` `=` $nested `>`";
}
+// Nested attributes for reproducing.
+def InternalAttr : Test_Attr<"Internal"> {
+ let mnemonic = "internal";
+
+ let parameters = (ins
+ "int64_t":$key,
+ "int64_t":$value
+ );
+
+ let assemblyFormat = "`<` struct(params) `>`";
+}
+
+def ExternalAttr : Test_Attr<"External"> {
+ let mnemonic = "external";
+
+ let parameters = (ins InternalAttr:$internal);
+
+ let assemblyFormat = "`<` struct(params) `>`";
+}
+
+def ExternalArrayAttr : Test_Attr<"ExternalArray"> {
+ let mnemonic = "external_array";
+
+ let parameters = (ins
+ ArrayRefParameter<"InternalAttr">:$internals
+ );
+
+ let assemblyFormat = "`<` `[` struct(params) `]` `>`";
+}
+
// An attribute testing AttributeSelfTypeParameter.
def AttrWithSelfTypeParam
: Test_Attr<"AttrWithSelfTypeParam", [TypedAttrInterface]> {
diff --git a/mlir/test/mlir-tblgen/op-format.mlir b/mlir/test/mlir-tblgen/op-format.mlir
index 08b0c52413a75..7d0bbd8a4273a 100644
--- a/mlir/test/mlir-tblgen/op-format.mlir
+++ b/mlir/test/mlir-tblgen/op-format.mlir
@@ -345,6 +345,27 @@ module attributes {test.someAttr = #test.cmpnd_nested_outer<i <42 <1, !test.smpl
//-----
+// CHECK: module attributes {test.internal = #test.internal<key = 8, value = 9>} {
+// CHECK-NEXT: }
+module attributes {test.internal = #test.internal<key = 8, value = 9>} {
+}
+
+//-----
+
+// CHECK: module attributes {test.external = #test.external<internal = <key = 1, value = 2>>} {
+// CHECK-NEXT: }
+module attributes {test.external = #test.external<internal = #test.internal<key = 1, value = 2>>} {
+}
+
+//-----
+
+// CHECK: module attributes {test.external_array = #test.external_array<[internals = <key = 1, value = 2>, <key = 8, value = 9>]>} {
+// CHECK-NEXT: }
+module attributes {test.external_array = #test.external_array<[internals = #test.internal<key = 1, value = 2>, #test.internal<key = 8, value = 9>]>} {
+}
+
+//-----
+
// CHECK: test.format_cpmd_nested_attr nested <i <42 <1, !test.smpla, [5, 6]>>>
test.format_cpmd_nested_attr nested <i <42 <1, !test.smpla, [5, 6]>>>
>From 39f57c0670e5457e42dd89d228c43fe239b54e1a Mon Sep 17 00:00:00 2001
From: Jueon Park <jueonpark at rebellions.ai>
Date: Thu, 9 Oct 2025 00:27:31 +0900
Subject: [PATCH 2/3] fix: add shouldBeQualifiedFlag and new printing utils for
corresponding cases
---
mlir/include/mlir/IR/OpImplementation.h | 38 +++++++++++++++++++
mlir/test/mlir-tblgen/op-format.mlir | 4 +-
.../tools/mlir-tblgen/AttrOrTypeFormatGen.cpp | 22 ++++++++---
3 files changed, 57 insertions(+), 7 deletions(-)
diff --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index d70aa346eaa1f..246ff72768242 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -20,6 +20,7 @@
#include "llvm/ADT/Twine.h"
#include "llvm/Support/SMLoc.h"
#include <optional>
+#include <type_traits>
namespace {
// reference https://stackoverflow.com/a/16000226
@@ -203,6 +204,43 @@ class AsmPrinter {
*this << attrOrType;
}
+ /// Print the provided attribute or type ensuring that any dialect-specific
+ /// prefixes (e.g. `#dialect.mnemonic`) are retained. This is used in
+ /// contexts, such as structured property dictionaries, where the fully
+ /// qualified form is required for disambiguation.
+ template <typename AttrOrType,
+ std::enable_if_t<std::is_convertible_v<AttrOrType, Attribute>> *
+ sfinae = nullptr>
+ void printQualifiedAttrOrType(AttrOrType attrOrType) {
+ printAttribute(attrOrType);
+ }
+
+ template <typename AttrOrType,
+ std::enable_if_t<std::is_convertible_v<AttrOrType, Type> &&
+ !std::is_convertible_v<AttrOrType, Attribute>> *
+ sfinae = nullptr>
+ void printQualifiedAttrOrType(AttrOrType attrOrType) {
+ printType(attrOrType);
+ }
+
+ template <typename ElementT,
+ std::enable_if_t<std::is_convertible_v<ElementT, Attribute> ||
+ std::is_convertible_v<ElementT, Type>> *sfinae =
+ nullptr>
+ void printQualifiedAttrOrType(ArrayRef<ElementT> attrOrTypes) {
+ llvm::interleaveComma(attrOrTypes, getStream(), [&](ElementT element) {
+ printQualifiedAttrOrType(element);
+ });
+ }
+
+ template <typename AttrOrType,
+ std::enable_if_t<!std::is_convertible_v<AttrOrType, Attribute> &&
+ !std::is_convertible_v<AttrOrType, Type>> *
+ sfinae = nullptr>
+ void printQualifiedAttrOrType(AttrOrType attrOrType) {
+ *this << attrOrType;
+ }
+
/// Print the given attribute without its type. The corresponding parser must
/// provide a valid type for the attribute.
virtual void printAttributeWithoutType(Attribute attr);
diff --git a/mlir/test/mlir-tblgen/op-format.mlir b/mlir/test/mlir-tblgen/op-format.mlir
index 1a3c7a7a818dc..f41e3d969b95c 100644
--- a/mlir/test/mlir-tblgen/op-format.mlir
+++ b/mlir/test/mlir-tblgen/op-format.mlir
@@ -352,14 +352,14 @@ module attributes {test.internal = #test.internal<key = 8, value = 9>} {
//-----
-// CHECK: module attributes {test.external = #test.external<internal = <key = 1, value = 2>>} {
+// CHECK: module attributes {test.external = #test.external<internal = #test.internal<key = 1, value = 2>>} {
// CHECK-NEXT: }
module attributes {test.external = #test.external<internal = #test.internal<key = 1, value = 2>>} {
}
//-----
-// CHECK: module attributes {test.external_array = #test.external_array<[internals = <key = 1, value = 2>, <key = 8, value = 9>]>} {
+// CHECK: module attributes {test.external_array = #test.external_array<[internals = #test.internal<key = 1, value = 2>, #test.internal<key = 8, value = 9>]>} {
// CHECK-NEXT: }
module attributes {test.external_array = #test.external_array<[internals = #test.internal<key = 1, value = 2>, #test.internal<key = 8, value = 9>]>} {
}
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
index 34547e9fed062..4ca93a78be370 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
@@ -141,6 +141,15 @@ class StructDirective
} // namespace
+/// Return true if the provided parameter should always be printed in qualified
+/// form (i.e., with dialect/type prefixes). Attribute and type parameters fall
+/// into this category to avoid ambiguity when nested within structured
+/// properties.
+static bool shouldPrintQualified(ParameterElement *param) {
+ StringRef cppType = param->getParam().getCppType();
+ return cppType.contains("Attr") || cppType.contains("Type");
+}
+
//===----------------------------------------------------------------------===//
// Format Strings
//===----------------------------------------------------------------------===//
@@ -155,7 +164,8 @@ static const char *const defaultParameterPrinter =
/// Qualified printer for attribute or type parameters: it does not elide
/// dialect and mnemonic.
-static const char *const qualifiedParameterPrinter = "$_printer << $_self";
+static const char *const qualifiedParameterPrinter =
+ "$_printer.printQualifiedAttrOrType($_self)";
/// Print an error when failing to parse an element.
///
@@ -849,13 +859,13 @@ static void guardOnAnyOptional(FmtContext &ctx, MethodBody &os,
}
void DefFormat::genCommaSeparatedPrinter(
- ArrayRef<FormatElement *> args, FmtContext &ctx, MethodBody &os,
+ ArrayRef<FormatElement *> params, FmtContext &ctx, MethodBody &os,
function_ref<void(FormatElement *)> extra) {
// Emit a space if necessary, but only if the struct is present.
if (shouldEmitSpace || !lastWasPunctuation) {
- bool allOptional = llvm::all_of(args, formatIsOptional);
+ bool allOptional = llvm::all_of(params, formatIsOptional);
if (allOptional)
- guardOnAnyOptional(ctx, os, args);
+ guardOnAnyOptional(ctx, os, params);
os << tgfmt("$_printer << ' ';\n", &ctx);
if (allOptional)
os.unindent() << "}\n";
@@ -864,7 +874,7 @@ void DefFormat::genCommaSeparatedPrinter(
// The first printed element does not need to emit a comma.
os << "{\n";
os.indent() << "bool _firstPrinted = true;\n";
- for (FormatElement *arg : args) {
+ for (FormatElement *arg : params) {
ParameterElement *param = getEncapsulatedParameterElement(arg);
if (param->isOptional()) {
param->genPrintGuard(ctx, os << "if (") << ") {\n";
@@ -872,6 +882,8 @@ void DefFormat::genCommaSeparatedPrinter(
}
os << tgfmt("if (!_firstPrinted) $_printer << \", \";\n", &ctx);
os << "_firstPrinted = false;\n";
+ if (param && shouldPrintQualified(param))
+ param->setShouldBeQualified();
extra(arg);
shouldEmitSpace = false;
lastWasPunctuation = true;
>From fb2bc05b52a957cf9e61d08b1bf2d574cdc42891 Mon Sep 17 00:00:00 2001
From: Jueon Park <jueonpark at rebellions.ai>
Date: Thu, 9 Oct 2025 16:19:12 +0900
Subject: [PATCH 3/3] fix: do not necessitate the internal attribute's mnemonic
---
mlir/include/mlir/IR/OpImplementation.h | 18 ++++++++++++++++--
mlir/test/mlir-tblgen/op-format.mlir | 8 ++++----
.../tools/mlir-tblgen/AttrOrTypeFormatGen.cpp | 19 +++++++++++++++++--
3 files changed, 37 insertions(+), 8 deletions(-)
diff --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index 246ff72768242..99e4db8355760 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -212,7 +212,14 @@ class AsmPrinter {
std::enable_if_t<std::is_convertible_v<AttrOrType, Attribute>> *
sfinae = nullptr>
void printQualifiedAttrOrType(AttrOrType attrOrType) {
- printAttribute(attrOrType);
+ Attribute attr = attrOrType;
+ Dialect &dialect = attr.getDialect();
+ StringRef dialectNamespace = dialect.getNamespace();
+ if (dialectNamespace.empty() || dialectNamespace == "builtin") {
+ printStrippedAttrOrType(attrOrType);
+ return;
+ }
+ printAttribute(attr);
}
template <typename AttrOrType,
@@ -220,7 +227,14 @@ class AsmPrinter {
!std::is_convertible_v<AttrOrType, Attribute>> *
sfinae = nullptr>
void printQualifiedAttrOrType(AttrOrType attrOrType) {
- printType(attrOrType);
+ Type type = attrOrType;
+ Dialect &dialect = type.getDialect();
+ StringRef dialectNamespace = dialect.getNamespace();
+ if (dialectNamespace.empty() || dialectNamespace == "builtin") {
+ printStrippedAttrOrType(attrOrType);
+ return;
+ }
+ printType(type);
}
template <typename ElementT,
diff --git a/mlir/test/mlir-tblgen/op-format.mlir b/mlir/test/mlir-tblgen/op-format.mlir
index f41e3d969b95c..60f8f2badef12 100644
--- a/mlir/test/mlir-tblgen/op-format.mlir
+++ b/mlir/test/mlir-tblgen/op-format.mlir
@@ -352,16 +352,16 @@ module attributes {test.internal = #test.internal<key = 8, value = 9>} {
//-----
-// CHECK: module attributes {test.external = #test.external<internal = #test.internal<key = 1, value = 2>>} {
+// CHECK: module attributes {test.external = #test.external<internal = <key = 1, value = 2>>} {
// CHECK-NEXT: }
-module attributes {test.external = #test.external<internal = #test.internal<key = 1, value = 2>>} {
+module attributes {test.external = #test.external<internal = <key = 1, value = 2>>} {
}
//-----
-// CHECK: module attributes {test.external_array = #test.external_array<[internals = #test.internal<key = 1, value = 2>, #test.internal<key = 8, value = 9>]>} {
+// CHECK: module attributes {test.external_array = #test.external_array<[internals = <key = 1, value = 2>, <key = 8, value = 9>]>} {
// CHECK-NEXT: }
-module attributes {test.external_array = #test.external_array<[internals = #test.internal<key = 1, value = 2>, #test.internal<key = 8, value = 9>]>} {
+module attributes {test.external_array = #test.external_array<[internals = <key = 1, value = 2>, <key = 8, value = 9>]>} {
}
//-----
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
index 4ca93a78be370..0c92f5035be09 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
@@ -146,8 +146,23 @@ class StructDirective
/// into this category to avoid ambiguity when nested within structured
/// properties.
static bool shouldPrintQualified(ParameterElement *param) {
- StringRef cppType = param->getParam().getCppType();
- return cppType.contains("Attr") || cppType.contains("Type");
+ const AttrOrTypeParameter ¶meter = param->getParam();
+ StringRef cppType = parameter.getCppType();
+ if (!cppType.contains("Attr") && !cppType.contains("Type"))
+ return false;
+
+ if (parameter.getPrinter())
+ return false;
+
+ if (const llvm::Init *init = parameter.getDef())
+ if (const auto *defInit = dyn_cast<llvm::DefInit>(init))
+ if (defInit->getDef()->isSubClassOf("EnumAttrInfo"))
+ return false;
+
+ if (cppType.contains("mlir::Attribute") || cppType.contains("mlir::Type"))
+ return true;
+
+ return false;
}
//===----------------------------------------------------------------------===//
More information about the Mlir-commits
mailing list