This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add support for interface inheritance
ClosedPublic

Authored by rriddle on Dec 16 2022, 2:42 AM.

Details

Summary

This allows for interfaces to define a set of "base classes",
which are interfaces whose methods/extra class decls/etc.
should be inherited by the derived interface. This more
easily enables combining interfaces and their dependencies,
without lots of awkard casting. Additional implicit conversion
operators also greatly simplify the conversion process.

One other aspect of this "inheritance" is that we also implicitly
add the base interfaces to the attr/op/type. The user can still
add them manually if desired, but this should help remove some
of the boiler plate when an interface has dependencies.

See https://discourse.llvm.org/t/interface-inheritance-and-dependencies-interface-method-visibility-interface-composition

Diff Detail

Event Timeline

rriddle created this revision.Dec 16 2022, 2:42 AM
rriddle requested review of this revision.Dec 16 2022, 2:42 AM
jpienaar added inline comments.Dec 29 2022, 1:19 PM
mlir/docs/Interfaces.md
667

instills?

669

I think this is something to expand on. What happens with "multiple inheritance" where there are overlapping method names?

mlir/include/mlir/IR/OpBase.td
2028

Should we default this list to be empty? (you've done that below but was wondering even here in case someone uses this raw base class)

mlir/include/mlir/Support/InterfaceSupport.h
231

Could you file github issue for this?

mlir/test/mlir-tblgen/op-interface.td
37

I think it would be good to have a unit test: the C++ API for usage and how inheritance works/convenience it affords I don't think is clear here or in the doc. A unit test I think would be great way to show how to use these.

mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
205

This is not strictly correct is it? E.g., we could be including this in a namespace with the same nesting below as exists at top level.

Generally looks good, +1 for some C++ test that exercises the behavior.

Thanks River!

rriddle updated this revision to Diff 488759.Jan 12 2023, 1:37 PM
rriddle edited the summary of this revision. (Show Details)
rriddle marked 5 inline comments as done.
rriddle added inline comments.Jan 12 2023, 1:37 PM
mlir/test/mlir-tblgen/op-interface.td
37

What do you have in mind exactly? The C++ aspect is already tested via TestInterfaces.td (it moves existing methods to be tested via inheritance). Examples are probably best left for docs (or in-tree examples, which the followup handles with FuncOpInterface).

mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
205

I didn't change this at all. If you don't drop it, stuff explodes:

llvm-project/build/tools/mlir/include/mlir/IR/OpAsmInterface.cpp.inc:71:1: error: no member named 'mlir' in 'llvm::StringRef'; did you mean simply 'mlir'?
::llvm::StringRef ::mlir::OpAsmOpInterface::getDefaultDialect() {
^~~~~~~~~~~~~~~~~~~~~~~~
mlir
llvm-project/mlir/include/mlir/IR/Verifier.h:12:11: note: 'mlir' declared here
namespace mlir {
jpienaar accepted this revision.Jan 14 2023, 9:29 AM

Thanks, LGTM

mlir/docs/Interfaces.md
669

Very nice examples. Could you add what happens if the interfaces have overlapping method names?

mlir/test/mlir-tblgen/op-interface.td
37

I was thinking of like hello-world C++ destruction order examples where one shows code and says what method will be actually called (throw in a couple of casts to show possible/inheritance gives you the Isa behavior of both etc). It's more an example that runs than anything else to show how fits together. You are correct follow up does add one, but was thinking this could be nice local example (you have expanded the docs a lot since so less of a need esp along with follow up).

This revision is now accepted and ready to land.Jan 14 2023, 9:29 AM
This revision was automatically updated to reflect the committed changes.