Interface inheritance and dependencies, interface method visibility, interface composition

MLIR
1

As we are starting to use more and more interface, I see some issues with scaling their use. So before we add any more interfaces and go down a certain path, I’d like to see whether others have encountered similar issues and whether we can distill some interface best practices.

interface dependencies

When splitting apart the StructuredOpInterface in linalg and extracting the more general concept of DestinationStyleOpInterface, we realized that after the splitting, the former interface ends up relying on the latter interface also being implemented. Currently, there is no way to encode this dependency other than mentioning it in a comment. Having dependencies defined in a more structured way would help with documentation and allow for improvements in generated code.

We cannot statically enforce dependencies, as interfaces on operations can also be implemented using external interfaces. So we can only decide the set of implemented interfaces at linktime, which practically means at runtime. But even checking at runtime in one place would be an improvement over code having to dyn_cast to an interface that is a dependency and handle failure.

We could go even further and make methods from interface dependencies also visible in the dependent interface, removing the need for casts in the first place. As a downside, this might lead to even more name clashes.

namespaces/scopes

Which brings me to the second issue: We are starting to see name clashed between interface methods and methods defined on the operation itself. For external interface implementations this is not an issue. However, if an operation directly implements an interfaces, the interface methods are visible for the operation itself, as well.

A recent example of this was in the DestinationStyleOpInterface that had a getInputs method. We resolved the conflict by renaming the method to getDPSInputs. However, adding ad-hoc namespaces to operations seems not like a good general solution.

Instead, we could consider to have interface implementations live in a different namespace/scope from regular methods (I will leave the C++ wizardry to make this happen to more qualified people). Doing so would lead to a couple more casts but at least it would always be clear which method is referenced.

partially implemented interfaces vs. many interfaces

In a recent review for an addition to the TilingInterface we had a discussion as to whether an interface should rather have methods that are optionally implemented (i.e. might just always fail for an operation) or whether one should split the interface instead.

My take is to go for the latter, as I quite like the idea that one can dyn_cast to an interface to check whether a certain operation, in principle, supports a behavior. Hence I suggested we have a TilingInterface and a PartialReductionInterface so that casting to the latter implies that the operation supports partial tiling of reductions.

The counter argument is that this leads to many smaller interfaces that jointly model behaviors needed for a transformation (like tiling and distributing in the above example). This adds boiler plate and makes navigating interfaces harder. I think this is manageable with documentation and tooling but would like to hear what others think.

3 Likes
2

This came up a bunch of times over the past 3 years.

It’d be great if we have reached a point of high enough entropy that we can finally address this properly!

Strong +1 to better core support here.

1 Like
3

Thanks for bringing this up, Stephan! Totally supportive of addressing these limitations.

Another option would be to make interface methods only accessible through the interface class and not through the op implementing the interface. In that way, we would always have to cast to the interface class to have access to the methods and would avoid name clashing in the op implementing the interface. A potential implementation could be to have the op implementing the interface define private methods like interfaceXMethodYImpl that would be accessible from the interface class via friendship.

This sounds like something to decide on a case by case basis. I personally don’t see a major problem in having a single interface where some methods have default implementations that bail out if no other implementation is provided.

4

On a separate but related note, I have also been struggling with some of these limitations and even a couple more:

To the interface dependencies limitation I would add that we currently can’t define interfaces with explicit cyclic dependencies. For example:

InterfaceA:
  InterfaceB methodX();

InterfaceB:
  InterfaceA methodY();

We touched on that limitation in ⚙ D136322 [mlir] ODS: emit interface model method at the end of the header. I think we would have to significantly change the way the interface code is generated.

Probably longer term but we have a few cases where having interfaces that can extend the operands of operations implementing the interface would simplify a lot the design of some features. One of those features is vector masking. This kind of interfaces would need to be designed carefully, though, to make sure we deal with def-use chains and other operand-operation queries properly.

5

Right now we already allow specifying a set of dependentTraits in tablegen. That wouldn’t directly work for interfaces given the dynamic aspect, but we having explicit interface dependencies seems relatively straightforward IMO.

What name clashes? When I’ve sketched out “interface inheritance” before the user facing aspect has always been somewhat similar to what people expect when they hear “inheritance”. It really comes down to how the shared methods are handled.

At a low-level, I would still expect each of the individual interfaces to be implemented explicitly for the op (with derived interfaces picking up the implementations required by the base ones). At the ODS level, my intention has been that “derived” interfaces would “imply” base ones. For example, FunctionOpInterface requires the op to be a symbol. We could have FunctionOpInterface imply SymbolOpInterface, so users only ever have to add FunctionOpInterface. That “imply” would require users to implement the necessary methods for SymbolOpInterface, and they could access those methods either via FunctionOpInterface or SymbolOpInterface, but the op only ever has one implementation of the symbol methods.

+1 here. This is very context dependent. The only thing here for me is that having an interface try to cover too much means that you have more “special cases” (and likely need to split).

I would strongly prefer to have the op define things. Having optional components based on interfaces is not worth the complexity that it would bring IMO.

– River

6

This also is unclear about what it means for interfaces that are externally loaded.

7

+1 on interface dependencies being somehow better supported

8

Thanks for the responses. Great to see that others are interested in this, too.

This is what I had in mind but could not articulate as well. Using interface prefixes in the method implementation names would be a neat trick to do this. Also, this would not change how things are used, except where current code relies on interface methods being visible on the operation itself. That is an anti-pattern anyway IMHO.

So, FunctionOpInterface would get auto-generated forwarding methods that do cast<SympolOpInterface>.foo(...)?

I was thinking that the more methods become visible in an interface, the more likely it is for names to clash. This likely is not an issue in practice, as one only depends on interfaces that are complimentary and hence likely have different method names.

I like what @dcaballe suggested for hiding interface methods on operations and reducing name clashes. And with the inheritance as suggested by @River707, usability would not suffer.

Do we also want to do some checking whether dependent interfaces are actually implemented? For interfaces on operations we can do this statically, for external interfaces, we could insert a runtime check with meaningful error message that triggers when the interface gets registered.

I agree, I was looking for more examples where one or the other direction was taken to learn about how others approach this.

9

I’m -1 on always doing this. There are plenty of interfaces that are purposefully describing fundamental aspects of an operation, it would be sad to have to effectively duplicate the methods being provided. I do however see value in making it explicit which things are intended to be exposed as part of the op API, and which things aren’t. I could likely see “namespacing” be the default, with explicit opt in for exporting into the public op API. I think realistically the methods being implemented by the op should always have the same form, otherwise it creates mass confusion of what the naming scheme is in what circumstances. We could always have forwarding methods generated for methods that want to be exposed as part of the public op API.

Conceptually, yes. There are different ways to slice it (depending on implementation). cast isn’t zero-cost on interfaces, so in my mind the derived interface would hold references to any base interfaces and directly dispatch.

For ODS it would be a requirement that the other interfaces are available (we already have some support for statically checking the op def for this). For external external interfaces, a runtime check seems valid (missing interfaces deps would generally represent a mis-config anyways, which could cause crashes/etc. if undetected)

– River

10

+1! This sounds pretty reasonable to me!

I think this deserves a separate discussion. We have cases, like vector masking and dynamic vector-length support, where those optional operands would need to be added to potentially any operation in MLIR. We currently have a design for masking that works and follows a different approach but it has some drawbacks that will significantly be accentuated when introducing support for dynamic vector lengths. We’ll bring this up for discussion as soon as we have some concrete ideas and examples. Of course, happy to get feedback and explore other options :slightly_smiling_face:

11

If the above sounds reasonable, I can look into implementing the things I described above in the next week or so.

– River

12

Sounds very reasonable to me. Thank you for taking this on!

13

Did you manage to make progress on this?
If so could you please update this thread with PRs ?

Thanks!

14

Sorry for the delay! I had an extra long US holiday and a bit swamped internally, but i should have an update here in the next week.

– River

2 Likes
15

Thanks River!

16

Starting sending out patches. Sent out interface inheritance sketch in ⚙ D140198 [mlir] Add support for interface inheritance (inherit methods, implicit conversions to base interfaces, etc. etc.). The visibility changes can come after that stack settles (they are significantly simpler changes to make either way).

– River

1 Like
17

Exciting, thank you for driving this River! This will help clean up a lot of uses of MLIR interfaces!