[mlir][TableGen] Emit interface traits after all interfaces #147699
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
Interface traits may provide default implementation of methods. When
this happens, the implementation may rely on another interface that is
not yet defined meaning that one gets "incomplete type" error during C++
compilation. In pseudo-code, the problem is the following:
```
InterfaceA has methodB() { return InterfaceB(); }
InterfaceB defined later
// What's generated is:
class InterfaceA { ... }
class InterfaceATrait {
// error: InterfaceB is an incomplete type
InterfaceB methodB() { return InterfaceB(); }
}
class InterfaceB { ... } // defined here
```
The two more "advanced" cases are:
* Cyclic dependency (A requires B and B requires A)
* Type-traited usage of an incomplete type (e.g.
`FailureOr<InterfaceB>`)
It seems reasonable to emit interface traits *after* all of the
interfaces have been defined to avoid the problem altogether.
As a drive by, make forward declarations of the interfaces early so that
user code does not need to forward declare.
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Andrei Golubev (andrey-golubev) ChangesInterface traits may provide default implementation of methods. When this happens, the implementation may rely on another interface that is not yet defined meaning that one gets "incomplete type" error during C++ compilation. In pseudo-code, the problem is the following: The two more "advanced" cases are:
It seems reasonable to emit interface traits after all of the interfaces have been defined to avoid the problem altogether. As a drive by, make forward declarations of the interfaces early so that user code does not need to forward declare. Full diff: https://github.com/llvm/llvm-project/pull/147699.diff 2 Files Affected:
diff --git a/mlir/test/lib/Dialect/Test/TestInterfaces.td b/mlir/test/lib/Dialect/Test/TestInterfaces.td
index dea26b8dda62a..d3d96ea5a65a4 100644
--- a/mlir/test/lib/Dialect/Test/TestInterfaces.td
+++ b/mlir/test/lib/Dialect/Test/TestInterfaces.td
@@ -174,4 +174,32 @@ def TestOptionallyImplementedTypeInterface
}];
}
+// Dummy type interface "A" that requires type interface "B" to be complete.
+def TestCyclicTypeInterfaceA : TypeInterface<"TestCyclicTypeInterfaceA"> {
+ let cppNamespace = "::mlir";
+ let methods = [
+ InterfaceMethod<"",
+ "::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceB>",
+ /*methodName=*/"returnB",
+ (ins),
+ /*methodBody=*/"",
+ /*defaultImpl=*/"return mlir::failure();"
+ >,
+ ];
+}
+
+// Dummy type interface "B" that requires type interface "A" to be complete.
+def TestCyclicTypeInterfaceB : TypeInterface<"TestCyclicTypeInterfaceB"> {
+ let cppNamespace = "::mlir";
+ let methods = [
+ InterfaceMethod<"",
+ "::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceA>",
+ /*methodName=*/"returnA",
+ (ins),
+ /*methodBody=*/"",
+ /*defaultImpl=*/"return mlir::failure();"
+ >,
+ ];
+}
+
#endif // MLIR_TEST_DIALECT_TEST_INTERFACES
diff --git a/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp b/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
index 4dfa1908b3267..ba1396e7d12be 100644
--- a/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
@@ -96,9 +96,9 @@ class InterfaceGenerator {
void emitConceptDecl(const Interface &interface);
void emitModelDecl(const Interface &interface);
void emitModelMethodsDef(const Interface &interface);
- void emitTraitDecl(const Interface &interface, StringRef interfaceName,
- StringRef interfaceTraitsName);
+ void forwardDeclareInterface(const Interface &interface);
void emitInterfaceDecl(const Interface &interface);
+ void emitInterfaceTraitDecl(const Interface &interface);
/// The set of interface records to emit.
std::vector<const Record *> defs;
@@ -445,9 +445,16 @@ void InterfaceGenerator::emitModelMethodsDef(const Interface &interface) {
os << "} // namespace " << ns << "\n";
}
-void InterfaceGenerator::emitTraitDecl(const Interface &interface,
- StringRef interfaceName,
- StringRef interfaceTraitsName) {
+void InterfaceGenerator::emitInterfaceTraitDecl(const Interface &interface) {
+ llvm::SmallVector<StringRef, 2> namespaces;
+ llvm::SplitString(interface.getCppNamespace(), namespaces, "::");
+ for (StringRef ns : namespaces)
+ os << "namespace " << ns << " {\n";
+
+ os << "namespace detail {\n";
+
+ StringRef interfaceName = interface.getName();
+ auto interfaceTraitsName = (interfaceName + "InterfaceTraits").str();
os << llvm::formatv(" template <typename {3}>\n"
" struct {0}Trait : public ::mlir::{2}<{0},"
" detail::{1}>::Trait<{3}> {{\n",
@@ -494,6 +501,10 @@ void InterfaceGenerator::emitTraitDecl(const Interface &interface,
os << tblgen::tgfmt(*extraTraitDecls, &traitMethodFmt) << "\n";
os << " };\n";
+ os << "}// namespace detail\n";
+
+ for (StringRef ns : llvm::reverse(namespaces))
+ os << "} // namespace " << ns << "\n";
}
static void emitInterfaceDeclMethods(const Interface &interface,
@@ -517,6 +528,19 @@ static void emitInterfaceDeclMethods(const Interface &interface,
os << tblgen::tgfmt(extraDecls->rtrim(), &extraDeclsFmt) << "\n";
}
+void InterfaceGenerator::forwardDeclareInterface(const Interface &interface) {
+ llvm::SmallVector<StringRef, 2> namespaces;
+ llvm::SplitString(interface.getCppNamespace(), namespaces, "::");
+ for (StringRef ns : namespaces)
+ os << "namespace " << ns << " {\n";
+
+ StringRef interfaceName = interface.getName();
+ os << "class " << interfaceName << ";\n";
+
+ for (StringRef ns : llvm::reverse(namespaces))
+ os << "} // namespace " << ns << "\n";
+}
+
void InterfaceGenerator::emitInterfaceDecl(const Interface &interface) {
llvm::SmallVector<StringRef, 2> namespaces;
llvm::SplitString(interface.getCppNamespace(), namespaces, "::");
@@ -533,7 +557,6 @@ void InterfaceGenerator::emitInterfaceDecl(const Interface &interface) {
if (!comments.empty()) {
os << comments << "\n";
}
- os << "class " << interfaceName << ";\n";
// Emit the traits struct containing the concept and model declarations.
os << "namespace detail {\n"
@@ -603,10 +626,6 @@ void InterfaceGenerator::emitInterfaceDecl(const Interface &interface) {
os << "};\n";
- os << "namespace detail {\n";
- emitTraitDecl(interface, interfaceName, interfaceTraitsName);
- os << "}// namespace detail\n";
-
for (StringRef ns : llvm::reverse(namespaces))
os << "} // namespace " << ns << "\n";
}
@@ -619,10 +638,15 @@ bool InterfaceGenerator::emitInterfaceDecls() {
llvm::sort(sortedDefs, [](const Record *lhs, const Record *rhs) {
return lhs->getID() < rhs->getID();
});
+ for (const Record *def : sortedDefs)
+ forwardDeclareInterface(Interface(def));
for (const Record *def : sortedDefs)
emitInterfaceDecl(Interface(def));
for (const Record *def : sortedDefs)
emitModelMethodsDef(Interface(def));
+ for (const Record *def : sortedDefs)
+ emitInterfaceTraitDecl(Interface(def));
+
return false;
}
|
|
@ftynse @joker-eph @River707 please take a look! I have stumbled upon this problem when patching one-shot bufferization and figured it makes sense to fix the behaviour instead of working around it. |
andrey-golubev
commented
Jul 9, 2025
| for (const Record *def : sortedDefs) | ||
| emitInterfaceDecl(Interface(def)); | ||
| for (const Record *def : sortedDefs) | ||
| emitModelMethodsDef(Interface(def)); | ||
| for (const Record *def : sortedDefs) | ||
| emitInterfaceTraitDecl(Interface(def)); |
There was a problem hiding this comment.
fyi: Interface ctor can be rather expensive (it has a vector of base interfaces that it populates + vector of methods).
As we re-create the objects 4 times here, does it make sense to refactor? (I failed to do it immediately so it'd probably go into a separate patch).
FWIW tablegen is part of the build system so if it is slow, the whole build is slow.
andrey-golubev
changed the title
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.
Interface traits may provide default implementation of methods. When this happens, the implementation may rely on another interface that is not yet defined meaning that one gets "incomplete type" error during C++ compilation. In pseudo-code, the problem is the following:
The two more "advanced" cases are:
FailureOr<InterfaceB>)It seems reasonable to emit interface traits after all of the interfaces have been defined to avoid the problem altogether.
As a drive by, make forward declarations of the interfaces early so that user code does not need to forward declare.