It sounds like you have some very specific negative personal experience here, which I haven’t experienced so there may be some something I’m missing.
That said, your opininon seems to be that we should have a very highly opinionated solution and try to “prevent” people from making mistakes. I don’t see that this approach could work in general, and I don’t see why it is valuable on balance with the major usability problem we have now, and the code duplication issue you’re suggesting as a solution.
I think we can be open to different communities in MLIR having different ways to solve problems. It is perfectly valid for one community to have a policy of “not registering any dialects in the tool” because they want perfectly isolated translations and use that as a way to ensure that. It seems also very valid for communities to go the other way and say “hey I want to be able to use a common set of dialects because … I write all my tests using them”.
It is also perfectly valid to have both in one project. You could have two tools: a “circt-translate-minimal” and “circt-translate-useful-for-writing-tests-against”. You could even have a “circt-translate -extra-dialects” flag and only conditionally register the dialects.
We aren’t taking away anything in the design space here.
I strongly object to this analogy though, as I explained it quite a few times in this thread already. I believe conflating *-translate with *-opt is ignoring fundamental differences.
As Stella mentioned it: mlir-translate is like “busy box” in how it dispatches to the translation. mlir-opt tools don’t have this behavior: instead in terms of comparison I see each of the “busy box” entry of mlir-translate more comparable to mlir-opt.
The problem is that it is not desirable that the way we provide control to impact one translation impacts them all.
Because in my own circt-translate, but also in mlir-translate, if I find the need to “allow-unregistered-dialects” by default on a subset of the translations, I shouldn’t have to enable it for all the translations. However that is what your API change forces the user into.
With your change the only way to segregate the behavior is to link multiple different tools. This breaks entirely the “busy box” dispatch.
It depends what the alternate path is: I understand what you’re trying to achieve, but I see your change as a quite disruptive with respect to the model right now.
You justified it with a hypothetical “generic translation”, we don’t have one upstream today so I’ll continue with this fictional use-case. I believe we could achieve similar behavior to what you’re describing in different ways. So let’s look at a hypothetical mlir-to-xml translation. The way we implement our current translation look like this:
void registerMlirToXMLTranslation() {
TranslateFromMLIRRegistration registration(
"mlir-to-xml",
[](ModuleOp module, raw_ostream &output) {
auto XML = translateModuleToXML(module);
if (!XML) return failure();
XML->print(output);
return success();
},
[](DialectRegistry ®istry) {
// what to put there?
// Maybe: `registry.allowUnregisteredDialect();` per Stella's suggestion?
// Or `registerAllDialects();` ? But then it forces everyone to link and enable all the upstream dialects...
});
}
So after your patch, the solution for enabling this in mlir-translate upstream would be to change its main() to register all the dialects and also allow unregistered dialects. But unfortunately that would apply to all the translations we have upstream
Edit: I think this is a critical point to answer, do we think it is just fine the day we add a “generic translation” upstream that mlir-translate will then just always registerAllDialects() and allow unregistered dialects for all upstream translations? How isn’t this making the DialectRegistry in the individual translation totally obsolete?
So I’d argue that a modeling that respects better the current layering would rather be to change the registration itself to be injectable:
// Register the `mlir-to-xml` translation **with a specific registry!**
void registerMlirToXMLTranslation(DialectRegistry registry) {
TranslateFromMLIRRegistration registration(
"mlir-to-xml",
[](ModuleOp module, raw_ostream &output) {
auto XML = translateModuleToXML(module);
if (!XML) return failure();
XML->print(output);
return success();
},
[registry](DialectRegistry ®) {
registry.appendTo(reg);
});
}
I believe that this still provides the feature we would want in terms of end-user, but it would preserve the isolation between each translation and allow to control them independently, so that for example controlling the behavior of mlir-translate --mlir-to-xml to accept unregistered dialects (or some specific dialects) does not “contaminate” the other translations (mlir-translate --llvmir-to-mlir for example).
Well, in the shoes of a user: if I implement --mlirfoo-to-bar in my translate tool: do I add my FooDialect in my tool’s main() or in the translation registration? Both achieve the same result: what’s the tradeoff between the two?
But also, beyond this, let’s say I have my own baz-translate tools and I want to experiment the generic --mlir-to-xml. If I use your new API to inject my FooDialect in the registry from main, this is what you intended it for I believe. However this leaks the FooDialect into all the translations.
Then later someone in the project/team implements a new translation: --mlirfoo-to-bar. They don’t add anything to the DialectRegistry in the translation registration API. They don’t have to think about it and won’t notice: it’ll work out of the box because of the “global context of the tool” which you created here.
What if I then want to cleanup our baz-translate because we don’t use --mlir-to-xml. I would remove my code and try to remove this “global registration of FooDialect” from the baz-translatemain(), because really it was added there to support this “generic translation”. However because it leaked in to this “global context”, we now implicitly depend on it from the --mlirfoo-to-bar translation and it’ll break.
So: this is breaking isolation between what should be “busy box” dispatches, creates a “global context” (uh…), and provides opportunities for added couplings between things that really shouldn’t be coupled.
To me in terms of design, proceeding forward with such properties requires cautious and comes with a fairly high bar with respect to alternative implementations as well as the pursued goal.
Just a thought: might the two of you (Chris and Mehdi) benefit from a private conversation about this? I’m a little bit surprised at the depth of passion being applied to this specific topic and knowing you both pretty well, I bet a f2f exchange would be more productive. We’ve got a lot bigger things to align on and I’m worried that you both waste your amazing brains on a topic of relatively minor importance
Mehdi and I had a chance to catch up, he’s totally right about the parameterized registration approach, I’ll give that a try. I’ll also look into adding the “allowUnregisteredDialect” bit to DialectRegistry and see what that ends up looking like.
Getting back to the original topic, maybe someone can update the docs with a crisp statement about what the *-translate tools are and aren’t so we can avoid having this discussion again
Had been meaning to do that for a while so we could fix some flag ergonomics better and (ultimately) exclude some things that are more in the developer tools category.
I have some experience with mlir-translate in two different contexts:
Conventional approach loading a file, translating its contents to MLIR
This was as straightforward as it sounds.
Bridge an existing graph compiler’s (not LLVM/MLIR-based) internal graph DS to MLIR.
This was an important use case where we wanted to take an existing project and transition it to MLIR - the graph was the chosen point to port to MLIR (from where we were able to quickly implement legalizations to TOSA as well).
The tricky part here was passing the graph thru llvm::sourceMgr at the TranslateToMLIRRegistration - we didn’t want to parse the input a second time, but instead translate the graph → MLIR which was a simpler exercise that I wrote too. After initially having our own binary for this, the better option was to have the sourceMgr path as the file reader and to invoke the graph builder there, then map to MLIR.
This also reminds me of a related issue with serialization registration - the existing infrastructure is fine for the purposes of a self-contained serialized representation, but an external form (e.g. flatbuffers) requires a conditional build rule if its part of a core dialect, to avoid the project-wide external dependency.
Just to confirm, Mehdi’s parameterized registration approach:
works great in practice, directly solving the problem I had. Thank you again Mehdi! I’ll take a look at putting “allow unregistered dialects” bit into DialectRegistry and prepare a phab to review.
IMO, it is primarily a compiler writer’s tool and even then only really useful for writing tests (for which it is really really useful!). Practical user tools will have a bunch of other considerations and don’t necessarily want to use the translation registry at all.
