I have the same hesitation. The interface concept is already quite complex and fragile. I’m also not sure what I think about deep interface hierarchies in this space.
I’m open to being incremental here and rallying to migrate Linalg itself. I just don’t want to leave it behind for long.
One way forward would be to create the new interface first. Then you can have an external model implementation of the interface for Linalg ops (like it is done for TilingInterface here. Should be easier ramp to switch Linalg ops over. Once it all works, you can make Linalg ops implement the interface by default.
I dont think we need Interface inheritance. The final state would be that Linalg ops implement the StructuredOpInterface and the DestinationPassingStyleInterface. In terms of verification, verification would need to check the latter interface first and then the former. Then the methods of the StructuredOpInterface can
LinalgOp linalgOp = .....
auto destinationPassingStlyeOp = cast<DestinationPassingStyleInterface>(linalgOp.getOperation());
to access the interface methods of the other interface.
In my local change, I have actually implemented this already that the Linalg ops implement both interfaces and I have added all the casts that are needed to the Linalg code. But there are a lot of casts needed, which made me think whether this is a good end state? I also like what Stephan suggested, that we have forwarding methods for the shared operations in the old interface. That means we would have the casts only in those forwarding methods instead of scattered all over the Linalg code base. But I also haven’t tried yet whether that would work. And maybe this also has some downsides that I cannot think of right now.
I think it is worth trying but I defer to @nicolasvasilache as the linalg codebase style arbitrator 
Also, this is not either or. We can fix all the use sites upstream, as you have done that work already, and still add forwarding methods that we mark deprecated to make this change more downstream friendly. Unless we want the forwarding to be permanent, of course.
Thanks for the mention I was not caught up in this thread.
The concept of interface inheritance has surfaced a bunch of times over the years and I have not seen particular interest to try and implement a good solution for such an approach.
I am wondering if interface composition is a viable approach: e.g. the following would automatically add all the methods of Interface2 and Interface3 inside Interface1.
OpInterface<Interface1, /*dependentInterfaces=*/[ Interface2, Interface3 ]>
Would that sound reasonable @aminim?
In the absence of that, casting seems reasonable to me, with the caveat that we have no hard guarantees that all ops that implement Interface1 also implement Interface2. Adding such a check in the verifier
I’d like to see some WIP PR to have an idea how intrusive casts are and whether manually forwarding functions make more sense right now.
This would help evaluate whether some larger restructuring would allow better separation between “DestinationPassingStyleOpInterface” and “StructuredOpInterface”.
Maybe some of the surface API of the interface can also be trimmed down.
Thanks for looking into this!
Here is a WIP PR that shows which files are currently using methods that would belong to the new interface. It compiles without asserts, to make it compile with asserts I would have to fix the casts (initially I thought I could directly case from linalgOp to the new interface and so the casts are missing the call to getOperation()).
Also, compared to what @pifon2a suggested, the isScalar method is currently part of the new interface because it is also used from other methods in the new interface. In this RFC it was suggested that it doesn’t have to be in any interface. But maybe it is possible to clean this up.
I think it makes sense to manually forward for now and put that in a clearly isolated block in the extra class declaration with a TODO.
If/when core infra provides a good way to automate this, we would then be able to just delete a dozen such 1-lines.
Thanks for sharing and for pushing on this @akuegel !
So just to confirm, this is how the forwarding would look like (example with the getNumInputs function):
//========================================================================//
// Forwarding functions to access interface methods from the
// DestinationStyleOpInterface.
//========================================================================//
int64_t getNumInputs() {
return cast<DestinationStyleOpInterface>(*this->getOperation())
.getNumInputs();
}
This would work for all users of LinalgOp, so we don’t need to add casts there. However when I tried it, I noticed I would still need to have separate casts for calls of these methods from the default implementations of the interface methods in LinalgStructuredInterface. But there are not so many of them, to be precise, 2 times call of getNumInputs, 1 times a call of getNumOutputs, and 1 times a call of getInputAndOutputOperands.
Yes, this makes sense to me, thanks for checking back!
The first step has landed, there is now a DestinationStyleOpInterface and the old LinalgStructuredInterface has forwarding methods.
I found one usage related to bufferization where we can almost switch from using LinalgOp to DestinationStyleOpInterface:
https://reviews.llvm.org/D132462
The only remaining method we still call from the old interface is payloadUsesValueFromOperand.
Is this an indication that we should possibly move it to the new interface as well? Also, does it actually have to be an interface method, or could it be a method as part of extraClassDeclaration (as far as I can tell, everyone is using the default implementation).
There are indeed several intertwined concepts that are referred to as interface inheritance. In this proposal, I think, it means that methods of InterfaceA can be implemented through methods of InterfaceB if the op already implements the latter (though it may not be a requirement for an op to implement InterfaceB in order to implement InterfaceA). To me, this sounds like a good fit for a trait, UseBToImplementA that provides the method implementations of InterfaceA through casts and methods of InterfaceB. The only caveat is to be careful with naming, if InterfaceA and InterfaceB have methods with identical names, things may go wrong with cryptic error messages or just unexpected behavior.
Some of the methods in the DestinationStyleOpInterface causes issues when moving ops to use this interface. If an operation has a method inputs, then the accessor for it is getInputs which clashes with the method of same name implemented in the interface. This was fixed ⚙ D136943 [mlir] Rename getInputs->getDpsInputs and getOutputs->getDpsInits in DPS interface. but looking at the methods of this interface, there seem to be methods that dont belong here. It might be better to remove these early before the interface gets more widespread usage. Here is a list of methods that we probably dont need to be in the interface
getDpsInputs, getDpsInputOperands, getDpsInput, isDpsInput : AFAIK, the DestinationStyleOpInterface is only concerned with specification of operands whose memory can be re-used for the result after bufferization, to make the bufferized operations in-place updates. The interface takes an opinionated stance of everything that is not a DpsOutput is an input. Interface itself should not concern itself with operands that are not DpsOutputs. I would suggest dropping these completely, and maybe just use a utility method which effectively returns all operands that are not DpsOutputs. I think that covers everything.
hasBufferSemantics and hasTensorSemantics. In Linalg these were use to check if an operation uses tensor operands or buffer operands. The implementation checked if all operands are memref types for hasBufferSemantics , and if all operands are tensor types for tensor semantics. I think there was at some point hope that we could have Linalg operations with mixed tensor and buffer semantics. Is that still relevant? If not, these methods could just be replaced with op->getNumResults() == 0 and op->getNumResults() != 0.
clone and cloneWithoutRegions : These are general utility methods that have nothing to do with Linalg or DestinationStyleOpInterface.
That would just leave the methods relating to getDpsInit in this interface…
RFC @nicolasvasilache and @pifon2a
@MaheshRavishankar I agree completely about clone and cloneWithoutRegions. These methods should be removed from the interface.
I am not sure what you mean by "could just be replaced with op->getNumResults... in the paragraph about hasBufferSemantics and hasTensorSemantics. Did you mean removing these methods from the interface or just replacing their implementation with the check for the number of results? I would prefer the latter, since the code is more readable. The rest of the checks like checking whether all inputs are tensors/scalars and all inits are tensors should just be in the verifier.
What is so opinionated about that? I guess the name. getDpsInputs returns "effectively all operands that are not DpsInits. I see no point in extracting it to a separate utility method.
The latter works for me. I was thinking we havent really pushed on having mixed semantics in Linalg ops (but I just saw a patch that used mixed semantics)
I still dont follow why Dps needs to know anything about inputs… if it is just “everything but outputs” then it doesnt even need to be an interface method
Seems like the renaming of outs to init is still pending? I can send a revision to do this: do we want to switch based on whether it is a tensor or a memref are we fine keeping inits for memref?
I’m bringing this back because seeing some people describing that we operate “in-place” on tensors: too much confusion going on with the mental model.
Haven’t heard anything, I’ll send a patch for this next week.
The last discussion I saw on this topic was that several groups were opposed to any more changes to the IR unless this was the last time and that any such plan should be dependent on an agreed upon style guide. I would expect a change of outs to inits would trigger the same blocking feedback.
Do you have a reference? See the discussion above, I didn’t see any such objection for this change in the discussion above?
This seems also very mechanism and scriptable.