[mlir] Introduce OpAsmAttrInterface for pretty-print #124721
Merged
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
|
@llvm/pr-subscribers-mlir-ods @llvm/pr-subscribers-mlir-core Author: Hongren Zheng (ZenithalHourlyRate) ChangesSee https://discourse.llvm.org/t/rfc-introduce-opasm-type-attr-interface-for-pretty-print-in-asmprinter/83792 for detailed introduction. This PR adds
In #121187 (comment) I mentioned splitting them into two PRs, but I realized that a PR with only definition of Cc @River707 @jpienaar @ftynse for review. Full diff: https://github.com/llvm/llvm-project/pull/124721.diff 7 Files Affected:
diff --git a/mlir/include/mlir/IR/CMakeLists.txt b/mlir/include/mlir/IR/CMakeLists.txt
index 0c7937dfd69e55..846547ff131e3a 100644
--- a/mlir/include/mlir/IR/CMakeLists.txt
+++ b/mlir/include/mlir/IR/CMakeLists.txt
@@ -2,6 +2,8 @@ add_mlir_interface(SymbolInterfaces)
add_mlir_interface(RegionKindInterface)
set(LLVM_TARGET_DEFINITIONS OpAsmInterface.td)
+mlir_tablegen(OpAsmAttrInterface.h.inc -gen-attr-interface-decls)
+mlir_tablegen(OpAsmAttrInterface.cpp.inc -gen-attr-interface-defs)
mlir_tablegen(OpAsmOpInterface.h.inc -gen-op-interface-decls)
mlir_tablegen(OpAsmOpInterface.cpp.inc -gen-op-interface-defs)
mlir_tablegen(OpAsmTypeInterface.h.inc -gen-type-interface-decls)
diff --git a/mlir/include/mlir/IR/OpAsmInterface.td b/mlir/include/mlir/IR/OpAsmInterface.td
index 34c830a12856fa..c3e84bccc5dee5 100644
--- a/mlir/include/mlir/IR/OpAsmInterface.td
+++ b/mlir/include/mlir/IR/OpAsmInterface.td
@@ -130,6 +130,28 @@ def OpAsmTypeInterface : TypeInterface<"OpAsmTypeInterface"> {
];
}
+//===----------------------------------------------------------------------===//
+// OpAsmAttrInterface
+//===----------------------------------------------------------------------===//
+
+def OpAsmAttrInterface : AttrInterface<"OpAsmAttrInterface"> {
+ let description = [{
+ This interface provides hooks to interact with the AsmPrinter and AsmParser
+ classes.
+ }];
+ let cppNamespace = "::mlir";
+
+ let methods = [
+ InterfaceMethod<[{
+ Get a name to use when generating an alias for this attribute.
+ }],
+ "::mlir::OpAsmDialectInterface::AliasResult", "getAlias",
+ (ins "::llvm::raw_ostream&":$os), "",
+ "return ::mlir::OpAsmDialectInterface::AliasResult::NoAlias;"
+ >,
+ ];
+}
+
//===----------------------------------------------------------------------===//
// ResourceHandleParameter
//===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index d9c925a9c56e6c..24e74e636fe51b 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -1824,6 +1824,7 @@ ParseResult parseDimensionList(OpAsmParser &parser,
//===--------------------------------------------------------------------===//
/// The OpAsmOpInterface, see OpAsmInterface.td for more details.
+#include "mlir/IR/OpAsmAttrInterface.h.inc"
#include "mlir/IR/OpAsmOpInterface.h.inc"
#include "mlir/IR/OpAsmTypeInterface.h.inc"
diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index fa4a1b4b72b024..cfac0a0d2f6474 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -125,6 +125,7 @@ void OpAsmPrinter::printFunctionalType(Operation *op) {
//===----------------------------------------------------------------------===//
/// The OpAsmOpInterface, see OpAsmInterface.td for more details.
+#include "mlir/IR/OpAsmAttrInterface.cpp.inc"
#include "mlir/IR/OpAsmOpInterface.cpp.inc"
#include "mlir/IR/OpAsmTypeInterface.cpp.inc"
@@ -1159,15 +1160,31 @@ template <typename T>
void AliasInitializer::generateAlias(T symbol, InProgressAliasInfo &alias,
bool canBeDeferred) {
SmallString<32> nameBuffer;
- for (const auto &interface : interfaces) {
- OpAsmDialectInterface::AliasResult result =
- interface.getAlias(symbol, aliasOS);
- if (result == OpAsmDialectInterface::AliasResult::NoAlias)
- continue;
- nameBuffer = std::move(aliasBuffer);
- assert(!nameBuffer.empty() && "expected valid alias name");
- if (result == OpAsmDialectInterface::AliasResult::FinalAlias)
- break;
+
+ OpAsmDialectInterface::AliasResult symbolInterfaceResult =
+ OpAsmDialectInterface::AliasResult::NoAlias;
+ if constexpr (std::is_base_of_v<Attribute, T>) {
+ if (auto symbolInterface = mlir::dyn_cast<OpAsmAttrInterface>(symbol)) {
+ symbolInterfaceResult = symbolInterface.getAlias(aliasOS);
+ if (symbolInterfaceResult !=
+ OpAsmDialectInterface::AliasResult::NoAlias) {
+ nameBuffer = std::move(aliasBuffer);
+ assert(!nameBuffer.empty() && "expected valid alias name");
+ }
+ }
+ }
+
+ if (symbolInterfaceResult != OpAsmDialectInterface::AliasResult::FinalAlias) {
+ for (const auto &interface : interfaces) {
+ OpAsmDialectInterface::AliasResult result =
+ interface.getAlias(symbol, aliasOS);
+ if (result == OpAsmDialectInterface::AliasResult::NoAlias)
+ continue;
+ nameBuffer = std::move(aliasBuffer);
+ assert(!nameBuffer.empty() && "expected valid alias name");
+ if (result == OpAsmDialectInterface::AliasResult::FinalAlias)
+ break;
+ }
}
if (nameBuffer.empty())
diff --git a/mlir/test/IR/op-asm-interface.mlir b/mlir/test/IR/op-asm-interface.mlir
index a9c199e3dc9736..5aa9bc7c5acaee 100644
--- a/mlir/test/IR/op-asm-interface.mlir
+++ b/mlir/test/IR/op-asm-interface.mlir
@@ -22,3 +22,18 @@ func.func @block_argument_name_from_op_asm_type_interface() {
}
return
}
+
+// -----
+
+//===----------------------------------------------------------------------===//
+// Test OpAsmAttrInterface
+//===----------------------------------------------------------------------===//
+
+// CHECK: #op_asm_attr_interface_test
+#attr = #test.op_asm_attr_interface<value = "test">
+
+func.func @test_op_asm_attr_interface() {
+ // CHECK-LABEL: @test_op_asm_attr_interface
+ %1 = "test.result_name_from_type"() {attr = #attr} : () -> !test.op_asm_type_interface
+ return
+}
\ No newline at end of file
diff --git a/mlir/test/lib/Dialect/Test/TestAttrDefs.td b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
index 0fd272f85d39bd..4b809c1c0a7656 100644
--- a/mlir/test/lib/Dialect/Test/TestAttrDefs.td
+++ b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
@@ -395,4 +395,14 @@ def TestCustomLocationAttr : Test_LocAttr<"TestCustomLocation"> {
let assemblyFormat = "`<` $file `*` $line `>`";
}
+// Test OpAsmAttrInterface.
+def TestOpAsmAttrInterfaceAttr : Test_Attr<"TestOpAsmAttrInterface",
+ [DeclareAttrInterfaceMethods<OpAsmAttrInterface, ["getAlias"]>]> {
+ let mnemonic = "op_asm_attr_interface";
+ let parameters = (ins "mlir::StringAttr":$value);
+ let assemblyFormat = [{
+ `<` struct(params) `>`
+ }];
+}
+
#endif // TEST_ATTRDEFS
diff --git a/mlir/test/lib/Dialect/Test/TestAttributes.cpp b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
index e09ea109061648..7c467308386f1f 100644
--- a/mlir/test/lib/Dialect/Test/TestAttributes.cpp
+++ b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
@@ -67,7 +67,7 @@ void CompoundAAttr::print(AsmPrinter &printer) const {
//===----------------------------------------------------------------------===//
Attribute TestDecimalShapeAttr::parse(AsmParser &parser, Type type) {
- if (parser.parseLess()){
+ if (parser.parseLess()) {
return Attribute();
}
SmallVector<int64_t> shape;
@@ -316,6 +316,17 @@ static ParseResult parseCustomFloatAttr(AsmParser &p, StringAttr &typeStrAttr,
return success();
}
+//===----------------------------------------------------------------------===//
+// TestOpAsmAttrInterfaceAttr
+//===----------------------------------------------------------------------===//
+
+::mlir::OpAsmDialectInterface::AliasResult
+TestOpAsmAttrInterfaceAttr::getAlias(::llvm::raw_ostream &os) const {
+ os << "op_asm_attr_interface_";
+ os << getValue().getValue();
+ return ::mlir::OpAsmDialectInterface::AliasResult::FinalAlias;
+}
+
//===----------------------------------------------------------------------===//
// Tablegen Generated Definitions
//===----------------------------------------------------------------------===//
|
|
@llvm/pr-subscribers-mlir Author: Hongren Zheng (ZenithalHourlyRate) ChangesSee https://discourse.llvm.org/t/rfc-introduce-opasm-type-attr-interface-for-pretty-print-in-asmprinter/83792 for detailed introduction. This PR adds
In #121187 (comment) I mentioned splitting them into two PRs, but I realized that a PR with only definition of Cc @River707 @jpienaar @ftynse for review. Full diff: https://github.com/llvm/llvm-project/pull/124721.diff 7 Files Affected:
diff --git a/mlir/include/mlir/IR/CMakeLists.txt b/mlir/include/mlir/IR/CMakeLists.txt
index 0c7937dfd69e55..846547ff131e3a 100644
--- a/mlir/include/mlir/IR/CMakeLists.txt
+++ b/mlir/include/mlir/IR/CMakeLists.txt
@@ -2,6 +2,8 @@ add_mlir_interface(SymbolInterfaces)
add_mlir_interface(RegionKindInterface)
set(LLVM_TARGET_DEFINITIONS OpAsmInterface.td)
+mlir_tablegen(OpAsmAttrInterface.h.inc -gen-attr-interface-decls)
+mlir_tablegen(OpAsmAttrInterface.cpp.inc -gen-attr-interface-defs)
mlir_tablegen(OpAsmOpInterface.h.inc -gen-op-interface-decls)
mlir_tablegen(OpAsmOpInterface.cpp.inc -gen-op-interface-defs)
mlir_tablegen(OpAsmTypeInterface.h.inc -gen-type-interface-decls)
diff --git a/mlir/include/mlir/IR/OpAsmInterface.td b/mlir/include/mlir/IR/OpAsmInterface.td
index 34c830a12856fa..c3e84bccc5dee5 100644
--- a/mlir/include/mlir/IR/OpAsmInterface.td
+++ b/mlir/include/mlir/IR/OpAsmInterface.td
@@ -130,6 +130,28 @@ def OpAsmTypeInterface : TypeInterface<"OpAsmTypeInterface"> {
];
}
+//===----------------------------------------------------------------------===//
+// OpAsmAttrInterface
+//===----------------------------------------------------------------------===//
+
+def OpAsmAttrInterface : AttrInterface<"OpAsmAttrInterface"> {
+ let description = [{
+ This interface provides hooks to interact with the AsmPrinter and AsmParser
+ classes.
+ }];
+ let cppNamespace = "::mlir";
+
+ let methods = [
+ InterfaceMethod<[{
+ Get a name to use when generating an alias for this attribute.
+ }],
+ "::mlir::OpAsmDialectInterface::AliasResult", "getAlias",
+ (ins "::llvm::raw_ostream&":$os), "",
+ "return ::mlir::OpAsmDialectInterface::AliasResult::NoAlias;"
+ >,
+ ];
+}
+
//===----------------------------------------------------------------------===//
// ResourceHandleParameter
//===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index d9c925a9c56e6c..24e74e636fe51b 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -1824,6 +1824,7 @@ ParseResult parseDimensionList(OpAsmParser &parser,
//===--------------------------------------------------------------------===//
/// The OpAsmOpInterface, see OpAsmInterface.td for more details.
+#include "mlir/IR/OpAsmAttrInterface.h.inc"
#include "mlir/IR/OpAsmOpInterface.h.inc"
#include "mlir/IR/OpAsmTypeInterface.h.inc"
diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index fa4a1b4b72b024..cfac0a0d2f6474 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -125,6 +125,7 @@ void OpAsmPrinter::printFunctionalType(Operation *op) {
//===----------------------------------------------------------------------===//
/// The OpAsmOpInterface, see OpAsmInterface.td for more details.
+#include "mlir/IR/OpAsmAttrInterface.cpp.inc"
#include "mlir/IR/OpAsmOpInterface.cpp.inc"
#include "mlir/IR/OpAsmTypeInterface.cpp.inc"
@@ -1159,15 +1160,31 @@ template <typename T>
void AliasInitializer::generateAlias(T symbol, InProgressAliasInfo &alias,
bool canBeDeferred) {
SmallString<32> nameBuffer;
- for (const auto &interface : interfaces) {
- OpAsmDialectInterface::AliasResult result =
- interface.getAlias(symbol, aliasOS);
- if (result == OpAsmDialectInterface::AliasResult::NoAlias)
- continue;
- nameBuffer = std::move(aliasBuffer);
- assert(!nameBuffer.empty() && "expected valid alias name");
- if (result == OpAsmDialectInterface::AliasResult::FinalAlias)
- break;
+
+ OpAsmDialectInterface::AliasResult symbolInterfaceResult =
+ OpAsmDialectInterface::AliasResult::NoAlias;
+ if constexpr (std::is_base_of_v<Attribute, T>) {
+ if (auto symbolInterface = mlir::dyn_cast<OpAsmAttrInterface>(symbol)) {
+ symbolInterfaceResult = symbolInterface.getAlias(aliasOS);
+ if (symbolInterfaceResult !=
+ OpAsmDialectInterface::AliasResult::NoAlias) {
+ nameBuffer = std::move(aliasBuffer);
+ assert(!nameBuffer.empty() && "expected valid alias name");
+ }
+ }
+ }
+
+ if (symbolInterfaceResult != OpAsmDialectInterface::AliasResult::FinalAlias) {
+ for (const auto &interface : interfaces) {
+ OpAsmDialectInterface::AliasResult result =
+ interface.getAlias(symbol, aliasOS);
+ if (result == OpAsmDialectInterface::AliasResult::NoAlias)
+ continue;
+ nameBuffer = std::move(aliasBuffer);
+ assert(!nameBuffer.empty() && "expected valid alias name");
+ if (result == OpAsmDialectInterface::AliasResult::FinalAlias)
+ break;
+ }
}
if (nameBuffer.empty())
diff --git a/mlir/test/IR/op-asm-interface.mlir b/mlir/test/IR/op-asm-interface.mlir
index a9c199e3dc9736..5aa9bc7c5acaee 100644
--- a/mlir/test/IR/op-asm-interface.mlir
+++ b/mlir/test/IR/op-asm-interface.mlir
@@ -22,3 +22,18 @@ func.func @block_argument_name_from_op_asm_type_interface() {
}
return
}
+
+// -----
+
+//===----------------------------------------------------------------------===//
+// Test OpAsmAttrInterface
+//===----------------------------------------------------------------------===//
+
+// CHECK: #op_asm_attr_interface_test
+#attr = #test.op_asm_attr_interface<value = "test">
+
+func.func @test_op_asm_attr_interface() {
+ // CHECK-LABEL: @test_op_asm_attr_interface
+ %1 = "test.result_name_from_type"() {attr = #attr} : () -> !test.op_asm_type_interface
+ return
+}
\ No newline at end of file
diff --git a/mlir/test/lib/Dialect/Test/TestAttrDefs.td b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
index 0fd272f85d39bd..4b809c1c0a7656 100644
--- a/mlir/test/lib/Dialect/Test/TestAttrDefs.td
+++ b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
@@ -395,4 +395,14 @@ def TestCustomLocationAttr : Test_LocAttr<"TestCustomLocation"> {
let assemblyFormat = "`<` $file `*` $line `>`";
}
+// Test OpAsmAttrInterface.
+def TestOpAsmAttrInterfaceAttr : Test_Attr<"TestOpAsmAttrInterface",
+ [DeclareAttrInterfaceMethods<OpAsmAttrInterface, ["getAlias"]>]> {
+ let mnemonic = "op_asm_attr_interface";
+ let parameters = (ins "mlir::StringAttr":$value);
+ let assemblyFormat = [{
+ `<` struct(params) `>`
+ }];
+}
+
#endif // TEST_ATTRDEFS
diff --git a/mlir/test/lib/Dialect/Test/TestAttributes.cpp b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
index e09ea109061648..7c467308386f1f 100644
--- a/mlir/test/lib/Dialect/Test/TestAttributes.cpp
+++ b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
@@ -67,7 +67,7 @@ void CompoundAAttr::print(AsmPrinter &printer) const {
//===----------------------------------------------------------------------===//
Attribute TestDecimalShapeAttr::parse(AsmParser &parser, Type type) {
- if (parser.parseLess()){
+ if (parser.parseLess()) {
return Attribute();
}
SmallVector<int64_t> shape;
@@ -316,6 +316,17 @@ static ParseResult parseCustomFloatAttr(AsmParser &p, StringAttr &typeStrAttr,
return success();
}
+//===----------------------------------------------------------------------===//
+// TestOpAsmAttrInterfaceAttr
+//===----------------------------------------------------------------------===//
+
+::mlir::OpAsmDialectInterface::AliasResult
+TestOpAsmAttrInterfaceAttr::getAlias(::llvm::raw_ostream &os) const {
+ os << "op_asm_attr_interface_";
+ os << getValue().getValue();
+ return ::mlir::OpAsmDialectInterface::AliasResult::FinalAlias;
+}
+
//===----------------------------------------------------------------------===//
// Tablegen Generated Definitions
//===----------------------------------------------------------------------===//
|
|
Ping for review |
River707
reviewed
Feb 4, 2025
mlir/lib/IR/AsmPrinter.cpp
Outdated
| OpAsmDialectInterface::AliasResult symbolInterfaceResult = | ||
| OpAsmDialectInterface::AliasResult::NoAlias; | ||
| if constexpr (std::is_base_of_v<Attribute, T>) { | ||
| if (auto symbolInterface = mlir::dyn_cast<OpAsmAttrInterface>(symbol)) { |
There was a problem hiding this comment.
Is there not a type variant of the getAlias method? I'd have expected parity for this method.
There was a problem hiding this comment.
The type variant of getAlias does exist but when we mlir::dyn_cast<OpAsmAttrInterface>(Type) MLIR will segfault so I guard it with if constexpr (std::is_base_of_v<Attribute, T>).
When another PR introducing getAlias method for OpAsmTypeInterface, the code here would be
if constexpr (std::is_base_of_v<Attribute, T>) {
if (auto symbolInterface = mlir::dyn_cast<OpAsmAttrInterface>(symbol)) {
...
}
} else if constexpr (std::is_base_of_v<Type, T>) {
if (auto symbolInterface = mlir::dyn_cast<OpAsmTypeInterface>(symbol)) {
...
}
}There was a problem hiding this comment.
See also #126364
There was a problem hiding this comment.
With the type interface supporting this as well, you should be able to do something like:
using InterfaceT = std::conditional_t<std::is_base_of_v<Attribute, T>, OpAsmAttrInterface, OpAsmTypeInterface>;
if (auto symbolInterface = dyn_cast<InterfaceT>(symbol)) {
...
}
Note also dropping mlir:: from the dyn_cast, mlir:: shouldn't be necessary in the majority of code upstream.
|
Ping for review |
1 similar comment
|
Ping for review |
River707
approved these changes
Feb 18, 2025
mlir/lib/IR/AsmPrinter.cpp
Outdated
| OpAsmDialectInterface::AliasResult symbolInterfaceResult = | ||
| OpAsmDialectInterface::AliasResult::NoAlias; | ||
| if constexpr (std::is_base_of_v<Attribute, T>) { | ||
| if (auto symbolInterface = mlir::dyn_cast<OpAsmAttrInterface>(symbol)) { |
There was a problem hiding this comment.
With the type interface supporting this as well, you should be able to do something like:
using InterfaceT = std::conditional_t<std::is_base_of_v<Attribute, T>, OpAsmAttrInterface, OpAsmTypeInterface>;
if (auto symbolInterface = dyn_cast<InterfaceT>(symbol)) {
...
}
Note also dropping mlir:: from the dyn_cast, mlir:: shouldn't be necessary in the majority of code upstream.
mlir/test/IR/op-asm-interface.mlir
Outdated
| #attr = #test.op_asm_attr_interface<value = "test"> | ||
|
|
||
| func.func @test_op_asm_attr_interface() { | ||
| // CHECK-LABEL: @test_op_asm_attr_interface |
There was a problem hiding this comment.
This check doesn't really add anything and can just be dropped.
ZenithalHourlyRate
force-pushed
the
opasm-attr-interface
branch
from
fb0eeaa to
da1447b
Compare
ZenithalHourlyRate
added a commit
that referenced
this pull request
Feb 18, 2025
See https://discourse.llvm.org/t/rfc-introduce-opasm-type-attr-interface-for-pretty-print-in-asmprinter/83792 for detailed introduction. This PR should be rebased once #124721 is merged. This PR adds * Definition of `getAlias` for `OpAsmTypeInterface` * Integration of `OpAsmTypeInterface` with `AsmPrinter` alias handling part This is partly in response to https://github.com/llvm/llvm-project/pull/124721/files#r1940399862 Cc @River707 for review.
wldfngrs
pushed a commit
to wldfngrs/llvm-project
that referenced
this pull request
Feb 19, 2025
See https://discourse.llvm.org/t/rfc-introduce-opasm-type-attr-interface-for-pretty-print-in-asmprinter/83792 for detailed introduction. This PR adds * Definition of `OpAsmAttrInterface` * Integration of `OpAsmAttrInterface` with `AsmPrinter` In llvm#121187 (comment) I mentioned splitting them into two PRs, but I realized that a PR with only definition of `OpAsmAttrInterface` is hard to test as it requires a custom Dialect with `OpAsmDialectInterface` to hook with `AsmPrinter`, so I just put them together to have a e2e test. Cc @River707 @jpienaar @ftynse for review.
wldfngrs
pushed a commit
to wldfngrs/llvm-project
that referenced
this pull request
Feb 19, 2025
See https://discourse.llvm.org/t/rfc-introduce-opasm-type-attr-interface-for-pretty-print-in-asmprinter/83792 for detailed introduction. This PR should be rebased once llvm#124721 is merged. This PR adds * Definition of `getAlias` for `OpAsmTypeInterface` * Integration of `OpAsmTypeInterface` with `AsmPrinter` alias handling part This is partly in response to https://github.com/llvm/llvm-project/pull/124721/files#r1940399862 Cc @River707 for review.
ZenithalHourlyRate
added a commit
that referenced
this pull request
Mar 5, 2025
… for alias (#128191) After the introduction of `OpAsmAttrInterface` for alias in #124721, the natural thought to exercise it would be migrating the MLIR existing alias generation method, i.e. `OpAsmDialectInterface`, to use the new interface. There is a `BuiltinOpAsmDialectInterface` that generates aliases for `AffineMapAttr` and `IntegerSetAttr`, and these attributes could be migrated to use `OpAsmAttrInterface`. However, the tricky part is that `OpAsmAttrInterface` lives in `OpImplementation.h`. If `BuiltinAttributes.h` includes that, it would become a cyclic inclusion. Note that only BuiltinAttribute/Type would face such issue as outside user can just include `OpImplementation.h` (see downstream example google/heir#1437) The dependency is introduced by the fact that `OpAsmAttrInterface` uses `OpAsmDialectInterface::AliasResult`. The solution to is: Put the `AliasResult` in `OpAsmSupport.h` that all interfaces can include that header safely. The API wont break as `mlir::OpAsmDialectInterface::AliasResult` is a typedef of this class.
This was referenced Mar 9, 2025
ZenithalHourlyRate
added a commit
that referenced
this pull request
May 12, 2025
…ation (#130479) After the introduction of `OpAsmAttrInterface`, it is favorable to migrate code using `OpAsmDialectInterface` for ASM alias generation, which lives in `Dialect.cpp`, to use `OpAsmAttrInterface`, which lives in `Attrs.td`. In this way, attribute behavior is placed near its tablegen definition and people won't need to go through other files to know what other (unexpected) hooks comes into play. See #124721 for the interface itself and #128191 for prior migration for Builtin Attributes. See #131504 for the `genMnemonicAlias` tablegen field.
ZenithalHourlyRate
added a commit
that referenced
this pull request
May 12, 2025
…neration (#130481) After the introduction of `OpAsmAttrInterface`, it is favorable to migrate code using `OpAsmDialectInterface` for ASM alias generation, which lives in `Dialect.cpp`, to use `OpAsmAttrInterface`, which lives in `Attrs.td`. In this way, attribute behavior is placed near its tablegen definition and people won't need to go through other files to know what other (unexpected) hooks comes into play. See #124721 for the interface itself and #128191 and #130479 for prior migrations. Note that `MLProgramOpAsmInterface` has no content now. However, if we delete it, a failure related to dialect resource handling will occur ``` within split at llvm-project/mlir/test/IR/invalid-file-metadata.mlir:60 offset :7:7: error: unexpected error: unexpected 'resource' section for dialect 'ml_program' ``` To support resource such interface must be registered.
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.
See https://discourse.llvm.org/t/rfc-introduce-opasm-type-attr-interface-for-pretty-print-in-asmprinter/83792 for detailed introduction.
This PR adds
OpAsmAttrInterfaceOpAsmAttrInterfacewithAsmPrinterIn #121187 (comment) I mentioned splitting them into two PRs, but I realized that a PR with only definition of
OpAsmAttrInterfaceis hard to test as it requires a custom Dialect withOpAsmDialectInterfaceto hook withAsmPrinter, so I just put them together to have a e2e test.Cc @River707 @jpienaar @ftynse for review.