[mlir][ODS] Fix TableGen for AttrOrTypeDef::hasStorageCustomConstructor #147957
+49
−2
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-core Author: Andrei Golubev (andrey-golubev) ChangesThere is a Full diff: https://github.com/llvm/llvm-project/pull/147957.diff 4 Files Affected:
diff --git a/mlir/test/lib/Dialect/Test/TestAttrDefs.td b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
index 4d825e2f0a8cc..382da592d0079 100644
--- a/mlir/test/lib/Dialect/Test/TestAttrDefs.td
+++ b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
@@ -431,4 +431,11 @@ def SlashAttr: Test_Attr<"Slash">{
let hasCustomAssemblyFormat = 1;
}
+def TestCustomStorageCtorAttr : Test_Attr<"TestCustomStorageCtorAttr"> {
+ let mnemonic = "custom_storage_ctor_attr";
+ let parameters = (ins "int":$value);
+ let assemblyFormat = "`<` $value `>`";
+ let hasStorageCustomConstructor = 1;
+}
+
#endif // TEST_ATTRDEFS
diff --git a/mlir/test/lib/Dialect/Test/TestAttributes.cpp b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
index 4f6655d0b2978..b31e90fc9ca91 100644
--- a/mlir/test/lib/Dialect/Test/TestAttributes.cpp
+++ b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
@@ -515,6 +515,18 @@ void SlashAttr::print(AsmPrinter &printer) const {
printer << "<" << getLhs() << " / " << getRhs() << ">";
}
+//===----------------------------------------------------------------------===//
+// TestCustomStorageCtorAttr
+//===----------------------------------------------------------------------===//
+
+test::detail::TestCustomStorageCtorAttrAttrStorage *
+test::detail::TestCustomStorageCtorAttrAttrStorage::construct(
+ mlir::StorageUniquer::StorageAllocator &, std::tuple<int> &&) {
+ // Note: this tests linker error ("undefined symbol"), the actual
+ // implementation is not important.
+ return nullptr;
+}
+
//===----------------------------------------------------------------------===//
// TestDialect
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/mlir-tblgen/attrdefs.td b/mlir/test/mlir-tblgen/attrdefs.td
index adec90dc5a371..d47411d6e860a 100644
--- a/mlir/test/mlir-tblgen/attrdefs.td
+++ b/mlir/test/mlir-tblgen/attrdefs.td
@@ -186,3 +186,16 @@ def I_TestGenMnemonicAliasAttr : TestAttr<"TestGenMnemonicAlias"> {
// DEF-NEXT: os << "test_gen_mnemonic_alias";
// DEF-NEXT: return ::mlir::OpAsmAliasResult::OverridableAlias;
// DEF-NEXT: }
+
+def J_CustomStorageCtorAttr : AttrDef<Test_Dialect, "CustomStorageCtorAttr"> {
+ let attrName = "test_custom_storage_ctor_attr";
+ let parameters = (ins "bool":$flag);
+ let hasStorageCustomConstructor = 1;
+}
+
+// Note: ';' at the end of construct method declaration is important - otherwise
+// one cannot provide custom definition
+
+// DEF-LABEL: struct CustomStorageCtorAttrAttrStorage : public ::mlir::AttributeStorage
+// DEF: static CustomStorageCtorAttrAttrStorage *construct
+// DEF-SAME: (::mlir::AttributeStorageAllocator &allocator, KeyTy &&tblgenKey);
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
index d9aa901ee2b28..dbae2143b920a 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
@@ -668,10 +668,10 @@ void DefGen::emitHashKey() {
}
void DefGen::emitConstruct() {
- Method *construct = storageCls->addMethod<Method::Inline>(
+ Method *construct = storageCls->addMethod(
strfmt("{0} *", def.getStorageClassName()), "construct",
def.hasStorageCustomConstructor() ? Method::StaticDeclaration
- : Method::Static,
+ : Method::StaticInline,
MethodParameter(strfmt("::mlir::{0}StorageAllocator &", valueType),
"allocator"),
MethodParameter("KeyTy &&", "tblgenKey"));
|
|
@llvm/pr-subscribers-mlir Author: Andrei Golubev (andrey-golubev) ChangesThere is a Full diff: https://github.com/llvm/llvm-project/pull/147957.diff 4 Files Affected:
diff --git a/mlir/test/lib/Dialect/Test/TestAttrDefs.td b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
index 4d825e2f0a8cc..382da592d0079 100644
--- a/mlir/test/lib/Dialect/Test/TestAttrDefs.td
+++ b/mlir/test/lib/Dialect/Test/TestAttrDefs.td
@@ -431,4 +431,11 @@ def SlashAttr: Test_Attr<"Slash">{
let hasCustomAssemblyFormat = 1;
}
+def TestCustomStorageCtorAttr : Test_Attr<"TestCustomStorageCtorAttr"> {
+ let mnemonic = "custom_storage_ctor_attr";
+ let parameters = (ins "int":$value);
+ let assemblyFormat = "`<` $value `>`";
+ let hasStorageCustomConstructor = 1;
+}
+
#endif // TEST_ATTRDEFS
diff --git a/mlir/test/lib/Dialect/Test/TestAttributes.cpp b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
index 4f6655d0b2978..b31e90fc9ca91 100644
--- a/mlir/test/lib/Dialect/Test/TestAttributes.cpp
+++ b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
@@ -515,6 +515,18 @@ void SlashAttr::print(AsmPrinter &printer) const {
printer << "<" << getLhs() << " / " << getRhs() << ">";
}
+//===----------------------------------------------------------------------===//
+// TestCustomStorageCtorAttr
+//===----------------------------------------------------------------------===//
+
+test::detail::TestCustomStorageCtorAttrAttrStorage *
+test::detail::TestCustomStorageCtorAttrAttrStorage::construct(
+ mlir::StorageUniquer::StorageAllocator &, std::tuple<int> &&) {
+ // Note: this tests linker error ("undefined symbol"), the actual
+ // implementation is not important.
+ return nullptr;
+}
+
//===----------------------------------------------------------------------===//
// TestDialect
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/mlir-tblgen/attrdefs.td b/mlir/test/mlir-tblgen/attrdefs.td
index adec90dc5a371..d47411d6e860a 100644
--- a/mlir/test/mlir-tblgen/attrdefs.td
+++ b/mlir/test/mlir-tblgen/attrdefs.td
@@ -186,3 +186,16 @@ def I_TestGenMnemonicAliasAttr : TestAttr<"TestGenMnemonicAlias"> {
// DEF-NEXT: os << "test_gen_mnemonic_alias";
// DEF-NEXT: return ::mlir::OpAsmAliasResult::OverridableAlias;
// DEF-NEXT: }
+
+def J_CustomStorageCtorAttr : AttrDef<Test_Dialect, "CustomStorageCtorAttr"> {
+ let attrName = "test_custom_storage_ctor_attr";
+ let parameters = (ins "bool":$flag);
+ let hasStorageCustomConstructor = 1;
+}
+
+// Note: ';' at the end of construct method declaration is important - otherwise
+// one cannot provide custom definition
+
+// DEF-LABEL: struct CustomStorageCtorAttrAttrStorage : public ::mlir::AttributeStorage
+// DEF: static CustomStorageCtorAttrAttrStorage *construct
+// DEF-SAME: (::mlir::AttributeStorageAllocator &allocator, KeyTy &&tblgenKey);
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
index d9aa901ee2b28..dbae2143b920a 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
@@ -668,10 +668,10 @@ void DefGen::emitHashKey() {
}
void DefGen::emitConstruct() {
- Method *construct = storageCls->addMethod<Method::Inline>(
+ Method *construct = storageCls->addMethod(
strfmt("{0} *", def.getStorageClassName()), "construct",
def.hasStorageCustomConstructor() ? Method::StaticDeclaration
- : Method::Static,
+ : Method::StaticInline,
MethodParameter(strfmt("::mlir::{0}StorageAllocator &", valueType),
"allocator"),
MethodParameter("KeyTy &&", "tblgenKey"));
|
There is a `hasStorageCustomConstructor` flag that allows one to provide custom attribute construction implementation. Unfortunately, it seems like the flag does not work properly: the generated C++ produces *empty body* method instead of producing only a declaration.
andrey-golubev
changed the title
joker-eph
reviewed
Jul 10, 2025
| strfmt("{0} *", def.getStorageClassName()), "construct", | ||
| def.hasStorageCustomConstructor() ? Method::StaticDeclaration | ||
| : Method::Static, | ||
| : Method::StaticInline, |
There was a problem hiding this comment.
Can we add a check in addMethod that detects the incompatibility between Method::Inline and StaticDeclaration?
There was a problem hiding this comment.
generally, i think it should work. i'll try locally. i guess if there's a bunch of failures, i'd prefer to put it into a separate patch.
btw, is it safe to assume then that Declaration implies "no definition"?
Method::InlineandStaticDeclaration
I guess you meant between Inline and Declaration? static is a separate property I imagine (e.g. same reasoning would apply to non-static methods also).
There was a problem hiding this comment.
apparently, this seems to be the only place within the upstream/main right now. anyway, pushed the "properties check" as a separate PR - #147979 (perhaps we need to figure out how we want to report the errors - only in debug mode or somehow differently).
joker-eph
approved these changes
Jul 10, 2025
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.
There is a
hasStorageCustomConstructorflag that allows one to provide custom attribute/type construction implementation. Unfortunately, it seems like the flag does not work properly: the generated C++ produces empty body method instead of producing only a declaration.