Skip to content

Conversation

zero9178
Copy link
Member

Using extraClassOf currently does not work with attribute or type interfaces as the generated code calls getInterfaceFor, a private method of AttributeInterface and TypeInterface respectively.

This PR fixes this by applying the same change that has been done to OpInterface in the past: Make getInterfaceFor a protected member of the class, allowing the generated code in subclasses to use it.

An attribute and type interface with extraClassOf have been added as interfaces in the test dialect to ensure it compiles without errors.

Using `extraClassOf` currently does not work with attribute or type interfaces as the generated code calls `getInterfaceFor`, a private method of `AttributeInterface` and `TypeInterface` respectively.

This PR fixes this by applying the same change that has been done to `OpInterface` in the past: Make `getInterfaceFor` a protected member of the class, allowing the generated code in subclasses to use it.

An attribute and type interface with `extraClassOf` have been added as interfaces in the test dialect to ensure it compiles without errors.
@zero9178 zero9178 requested review from a team as code owners September 13, 2023 21:27
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Sep 13, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 13, 2023

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Changes Using `extraClassOf` currently does not work with attribute or type interfaces as the generated code calls `getInterfaceFor`, a private method of `AttributeInterface` and `TypeInterface` respectively.

This PR fixes this by applying the same change that has been done to OpInterface in the past: Make getInterfaceFor a protected member of the class, allowing the generated code in subclasses to use it.

An attribute and type interface with extraClassOf have been added as interfaces in the test dialect to ensure it compiles without errors.

Full diff: https://github.com/llvm/llvm-project/pull/66292.diff

3 Files Affected:

  • (modified) mlir/include/mlir/IR/Attributes.h (+1-1)
  • (modified) mlir/include/mlir/IR/Types.h (+1-1)
  • (modified) mlir/test/lib/Dialect/Test/TestInterfaces.td (+26)
diff --git a/mlir/include/mlir/IR/Attributes.h b/mlir/include/mlir/IR/Attributes.h
index 53a2a1f2ad59f9e..ac5dd49f0ba3970 100644
--- a/mlir/include/mlir/IR/Attributes.h
+++ b/mlir/include/mlir/IR/Attributes.h
@@ -291,7 +291,7 @@ class AttributeInterface
                                           Attribute, AttributeTrait::TraitBase>;
   using InterfaceBase::InterfaceBase;
 
-private:
+protected:
   /// Returns the impl interface instance for the given type.
   static typename InterfaceBase::Concept *getInterfaceFor(Attribute attr) {
 #ifndef NDEBUG
diff --git a/mlir/include/mlir/IR/Types.h b/mlir/include/mlir/IR/Types.h
index 8443518027c0b62..832794928a44198 100644
--- a/mlir/include/mlir/IR/Types.h
+++ b/mlir/include/mlir/IR/Types.h
@@ -276,7 +276,7 @@ class TypeInterface : public detail::Interface<ConcreteType, Type, Traits, Type,
       detail::Interface<ConcreteType, Type, Traits, Type, TypeTrait::TraitBase>;
   using InterfaceBase::InterfaceBase;
 
-private:
+protected:
   /// Returns the impl interface instance for the given type.
   static typename InterfaceBase::Concept *getInterfaceFor(Type type) {
 #ifndef NDEBUG
diff --git a/mlir/test/lib/Dialect/Test/TestInterfaces.td b/mlir/test/lib/Dialect/Test/TestInterfaces.td
index e2ed27bdf0203e3..79a6c86493d418c 100644
--- a/mlir/test/lib/Dialect/Test/TestInterfaces.td
+++ b/mlir/test/lib/Dialect/Test/TestInterfaces.td
@@ -147,4 +147,30 @@ def TestOptionallyImplementedOpInterface
   }];
 }
 
+def TestOptionallyImplementedAttrInterface
+    : AttrInterface<"TestOptionallyImplementedAttrInterface"> {
+  let cppNamespace = "::mlir";
+
+  let methods = [
+    InterfaceMethod<"", "bool", "getImplementsInterface", (ins)>,
+  ];
+
+  let extraClassOf = [{
+    return $_attr.getImplementsInterface();
+  }];
+}
+
+def TestOptionallyImplementedTypeInterface
+    : TypeInterface<"TestOptionallyImplementedTypeInterface"> {
+  let cppNamespace = "::mlir";
+
+  let methods = [
+    InterfaceMethod<"", "bool", "getImplementsInterface", (ins)>,
+  ];
+
+  let extraClassOf = [{
+    return $_type.getImplementsInterface();
+  }];
+}
+
 #endif // MLIR_TEST_DIALECT_TEST_INTERFACES

Copy link
Member

@jpienaar jpienaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable and consistent with previous.

@zero9178 zero9178 merged commit be59265 into llvm:main Sep 13, 2023
@zero9178 zero9178 deleted the attr-type-classof branch September 13, 2023 21:38
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
…llvm#66292)

Using `extraClassOf` currently does not work with attribute or type
interfaces as the generated code calls `getInterfaceFor`, a private
method of `AttributeInterface` and `TypeInterface` respectively.

This PR fixes this by applying the same change that has been done to
`OpInterface` in the past: Make `getInterfaceFor` a protected member of
the class, allowing the generated code in subclasses to use it.

An attribute and type interface with `extraClassOf` have been added as
interfaces in the test dialect to ensure it compiles without errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants