[mlir] Declare promised interfaces for all dialects #78368
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
justinfargnoli
changed the title
|
Open question: where should I declare promised interfaces for the |
justinfargnoli
changed the title
justinfargnoli
force-pushed
the
jf/promise-interfaces
branch
2 times, most recently
from
22d1a01
to
41811e4
Compare
justinfargnoli
requested review from
aartbik,
PeimingLiu,
yinying-lisa-li,
matthias-springer,
dcaballe and
nicolasvasilache
as code owners
llvmbot
added
mlir:linalg
mlir:gpu
mlir:affine
mlir:vectorops
mlir:sparse
@zero9178, thank you for the detailed write-up. I was only able to reproduce the following missing dependency:
Add Please let me know if you're still able to reproduce the additional missing dependencies on your end. |
@ftynse, assuming I've correctly followed the discussion, Update include should address this. |
| @@ -63,6 +64,8 @@ void mlir::bufferization::BufferizationDialect::initialize() { | |||
| #include "mlir/Dialect/Bufferization/IR/BufferizationOps.cpp.inc" | |||
| >(); | |||
| addInterfaces<BufferizationInlinerInterface>(); | |||
| declarePromisedInterfaces<BufferizableOpInterface, func::CallOp, func::FuncOp, | |||
| func::ReturnOp>(); | |||
There was a problem hiding this comment.
This should be in the Func dialect I believe
There was a problem hiding this comment.
Just to confirm, is this reasoning incorrect?
I asked this because I wasn't sure if you were responding to the earlier discussion, but I'm realizing now that you're just responding to my ping from yesterday.
This should be in the Func dialect I believe
Thus, this sounds good to me. I'll make the change.
There was a problem hiding this comment.
It is sufficient to put this in the func dialect for now, and eventually proceed with splitting out the interface from the dialect as I did with transform if feasible/desirable.
There was a problem hiding this comment.
Move Func Op declared interfaces to the Func Dialect code. should address this.
| #include "mlir/Dialect/Complex/IR/Complex.h" | ||
| #include "mlir/Dialect/Tensor/IR/Tensor.h" | ||
| #include "mlir/Dialect/Transform/IR/TransformInterfaces.h" |
There was a problem hiding this comment.
#85221 achieves the split. It is mostly orthogonal to this patch, so no need to wait for that one to land IMO.
| @@ -63,6 +64,8 @@ void mlir::bufferization::BufferizationDialect::initialize() { | |||
| #include "mlir/Dialect/Bufferization/IR/BufferizationOps.cpp.inc" | |||
| >(); | |||
| addInterfaces<BufferizationInlinerInterface>(); | |||
| declarePromisedInterfaces<BufferizableOpInterface, func::CallOp, func::FuncOp, | |||
| func::ReturnOp>(); | |||
There was a problem hiding this comment.
It is sufficient to put this in the func dialect for now, and eventually proceed with splitting out the interface from the dialect as I did with transform if feasible/desirable.
|
Assuming the tests pass and I don't receive any other comments, I'll plan on merging this over the weekend! |
|
Apart from the obvious layering issues this change has, it also makes it impossible to mix and match bufferization interface implementations, which users actually do. Can we revert this? The generic interfaces seem fine, but the bufferization dialect changes seem to be fundamentally incompatible with MLIR's design and tightly couple things that shouldn't be coupled. |
I can't figure out what do you mean by that? Can you clarify what "mix and match" refer to more precisely? Maybe give an example?
Can you clarify where do you see a coupling right now? The only intended coupling is that the system should be "well setup": without promises it is just too brittle. Actually the addition of external interface was not supposed to be added without this mechanism, this has just been a ball dropped on the follow-ups there for too long. |
Right now you can use parts of bufferization. Like bufferizing my custom dialect but don't bufferize all of the other MLIR dialects (which comes with its own assumptions on Tensors and MemRefs). After this change you're forced to load all the bufferization interfaces for all the ops, which breaks this use case.
It couples Tensor to Bufferization and to Transform dialect. While technically this can be made a header-only dependency (right now it's not), I really dislike this from a design standpoint. MLIR is meant to be used as parts and now there's a dependency between dialects that are meant to be independent. I like the intention of making interfaces less fragile to use, but the point of having the external interfaces in the first place is allowing us to have weaker coupling. |
The promise declarations are meant to be header only, why is it not right now?
The promise only forces you to load what you use right now: are you saying that you have ops in your IR for which you want to run the pass but you want the |
It does depend on TypeIDs to be present.
I guess I can make it work through Bufferization's OpFilter, but that seems backwards to me. Why do I have to first load an extension interface just to not use it? Shouldn't I only load what I actually want to use? |
If Bufferization is doing
Right we get a weak symbol injected, but that's still header-only to me, why do you consider it's not? |
That's not what
I don't consider those header-only dependencies, and there are build modes where this will break. The implementation is fine to give those symbols a home. But I guess so far MLIR has been fine with it so maybe it's not a big deal. |
I would say tentatively yes based on what I understand OpFiltef to be about, but let’s ask @matthias-springer about the intended behavior for OpFilter?
I am interested to understand this more: what are these build modes? And why aren’t these « header-only » to you? (IIRC we crafted all this around inline functions and rely on ODR rules) |
Clang's -fmodules-codegen is what I had in mind. Also windows dlls, but that's enough of a minefield not to care about. |
Sent #85690 that fixes the issue I'm seeing, thanks. |
|
Personally, I think this line of work is ballooning in a way that needs a broader discussion and perhaps needs to be promoted to an ODM. I'm seeing a lot of objections and we are continuing past them with assertions that it is fine. It is possible/likely that this is a group misunderstanding of a necessarily tricky thing, but I still think that warrants discussion vs just proceeding. |
|
I haven’t seen any objection that is warranting a revert right now, but we can continue discussing at an ODM of course. |
|
Then I will be more explicit, whereas I was giving a benefit of the doubt: I think this needs additional design discussion before we add a new kind of dependency edge like this to the codebase. With all thanks to those who are trying to get this done, it increases the cognitive load on all of us and I need to understand the motivations and implications more clearly. (I'm using "I" words here but I don't think I'm the only one who is running at an understanding deficit on this) |
You have to elaborate on what do you call "Dependency edge" exactly...
This is all a fallout from adding "external interfaces" to quickly actually. |
I agree with that. I think we need to take a pause and evaluate. Once added, these things live for a very long time, and I think we owe it to ourselves if we are feeling discomfort about the feature direction to step back and make sure we've got a good view of it. This isn't a knock on anyone: these things can be really tricky to get right, and they can balloon far outside of the original conceived scope. We can stop the line on that and discuss vs just moving forward. Speaking again just for myself, I'd be willing/able to give it more principled thought but can't get my brain around it mid flight like this (without a pause/discussion). |
This header only "backwards" dep on the TypeID. Multiple people on this thread look at this and think it is a layering violation -- not just in some specific build system senses but in the "are we sure we want to do that" sense. |
|
Assuming we have completeness on the "promises" right now, pausing on any new addition seems fine to me, but that will block anyone who has a new interface use upstream. That is, I would object to "backslide" from a state where we enforce our invariants right now, by adding new "unsafe" interfaces registrations in tree. |
|
I'm personally fine with us raising the bar on such architectural changes upstream. Not sure I would say it is all or nothing, but I think we have gotten somewhat ahead of ourselves on interfaces and deps among these dialects. Some back pressure forcing more communication and thought wouldn't be a bad thing, imo. |
| declarePromisedInterfaces<bufferization::BufferizableOpInterface, BranchOp, | ||
| CondBranchOp>(); | ||
| declarePromisedInterface<CondBranchOp, | ||
| bufferization::BufferDeallocationOpInterface>(); |
There was a problem hiding this comment.
I share some concerns about the layering here. This seems to tightly couple bufferization with potentially unrelated dialects.
We have a pass and test downstream in IREE that started crashing after this change:
- Test: https://github.com/openxla/iree/blob/main/tests/e2e/regression/force_single_dispatch.mlir
- Pass: https://github.com/openxla/iree/blob/main/compiler/src/iree/compiler/Preprocessing/Common/MakeSingleDispatchForFunction.cpp
- Logs: https://github.com/openxla/iree/actions/runs/8425984714/job/23073325026?pr=16891#step:5:2974
As far as I can tell, that code should not be using bufferization.
There was a problem hiding this comment.
Possible false alarm on "that code should not be using bufferization" after some further debugging... but the layering here still feels inverted to me.
There was a problem hiding this comment.
From the backtrace, the crash you have seems actually a good showcase for "incorrect setup" detected by the promise right? You're using the bufferization interface but "forgot" to register the extension for the CF dialect as far as I can tell.
There was a problem hiding this comment.
Yep, fixed the crash with iree-org/iree@24ee045
A pointer to the register functions to use in the PR description could have helped speed up that triage/fix process.
This PR adds promised interface declarations for all interfaces declared in
InitAllDialects.h.Promised interfaces allow a dialect to declare that it will have an implementation of a particular interface, crashing the program if one isn't provided when the interface is used.