In the previous state, we were relying on forcing the linker to include
all libraries in the final binary and the global initializer to self-register
every piece of the system. This change help moving away from this model, and
allow users to compose pieces more freely. The current change is only "fixing"
the dialect registration and avoiding relying on "whole link" for the passes.
The translation is still relying on the global registry, and some refactoring
is needed to make this all more convenient.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/tools/mlir-opt/CMakeLists.txt | ||
---|---|---|
99 | ^^ This was the goal for this milestone |
Yay, thanks good step in the right direction.
mlir/include/mlir/InitAllPasses.h | ||
---|---|---|
40 | s/must be called/may be called/ s/force registering the/register all/ ? | |
46 | command-line testing tools ? | |
49 | Sentence fragment ? (, at end) | |
53 | But you've now documented it inside the compiler, what if the compiler reads its own comments?!? | |
71 | Do you want to add somewhere a comment as to the values/should these have flags associated and should those flags be in this file ... (can also be follow up if so :)) | |
mlir/include/mlir/Transforms/Passes.h | ||
113–114 | list of ops ? | |
mlir/lib/Dialect/CMakeLists.txt | ||
31 | I don't know what this does? |
mlir/include/mlir/InitAllPasses.h | ||
---|---|---|
40 | Use /// for comments. | |
48 | nit: Drop the newline here. | |
81 | I thought this was removed? | |
mlir/lib/Quantizer/Transforms/InferQuantizedTypesPass.cpp | ||
287 | What are you moving pass registration to? | |
mlir/test/Dialect/SPIRV/TestAvailability.cpp | ||
217 | Can you not do void mlir::registerConvertToTargetEnvPass? |
Add back FULL_LIBS to mlir-translate, inadvertently removed between the two diffs
mlir/include/mlir/InitAllPasses.h | ||
---|---|---|
71 | You noticed that this call will never be executed right? | |
mlir/lib/Dialect/CMakeLists.txt | ||
31 | Added a comment: Empty libraries aren't possible with CMake, create a dummy file. | |
mlir/lib/Quantizer/Transforms/InferQuantizedTypesPass.cpp | ||
287 | At the moment we just removed the "force link" invocation in favor of pulling it objects by referencing their symbols. Ultimately I'd like this to replace the static below though. I couldn't use a "createXXXXPass" method here because the pass needs a complex data structure in its initializer. | |
mlir/test/Dialect/SPIRV/TestAvailability.cpp | ||
217 | I would need a header declaration for this, but we don't have public headers for the test directory |
I think getting rid of whole_archive_link is a good thing, but I'm concerned that this actually makes it somewhat more complicated to get new Passes and Dialects into the system in a not error-prone way.
mlir/examples/toy/Ch7/toyc.cpp | ||
---|---|---|
23 | Why "Init" vs. "register"? A little bikeshedding, but when it's this close, it's really annoying that it's not exactly the same.. | |
mlir/include/mlir/InitAllPasses.h | ||
18–30 | Most passes are associated with a Dialect, or the presence of two related Dialects. Perhaps this could be used to simplify this registration? | |
56 | It seems like there should be some hierarchy here. I don't think it scales to list all the passes here. At least the ones from a dialect should be 'registered' as part of the dialect registration? | |
mlir/test/EDSC/builder-api-test.cpp | ||
40 | maybe registerDialects could be a variadic template class, avoiding the need for the functor? |
to get new Passes and Dialects into the system in a not error-prone way
What kind of error that wouldn’t be trivially found during ninja check do you foresee?
In general I agree with all you other comments in this revision, you can see this as an incremental step. I had to land this because I couldn’t get more done and it was important to fix the nasty issue with incremental builds of mlir-opt being broken.
I’d like indeed to look into more unified way of registering passes with their dialect for example.
For the long list of passes, have you seen how LLVM works?
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/LinkAllPasses.h
I did it shamelessly here only because LLVM was as pragmatic... I don’t find it particularly nice.
mlir/lib/Dialect/CMakeLists.txt | ||
---|---|---|
36–52 | Do you actually want this library to exist, or just to have an easy way to structure CMakeFiles? |
mlir/lib/Dialect/CMakeLists.txt | ||
---|---|---|
36–52 | Do you mean add_mlir_dialect would build a CMake macro like "ALL_MLIR_DIALECTS" and this macro would be used to link them all in place where we use MLIRAllDialects right now? That works for me! |
Why "Init" vs. "register"? A little bikeshedding, but when it's this close, it's really annoying that it's not exactly the same..