This is an archive of the discontinued LLVM Phabricator instance.

Remove static registration for dialects, and the "alwayslink" hack for passes
ClosedPublic

Authored by mehdi_amini on Feb 11 2020, 9:08 PM.

Details

Summary

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.

Diff Detail

Event Timeline

mehdi_amini created this revision.Feb 11 2020, 9:08 PM
mehdi_amini marked an inline comment as done.Feb 11 2020, 9:17 PM
mehdi_amini added inline comments.
mlir/tools/mlir-opt/CMakeLists.txt
99

^^ This was the goal for this milestone

jpienaar accepted this revision.Feb 11 2020, 9:26 PM
jpienaar marked an inline comment as done.

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?

This revision is now accepted and ready to land.Feb 11 2020, 9:26 PM
rriddle accepted this revision.Feb 11 2020, 11:19 PM
rriddle added inline comments.
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?

mehdi_amini marked 14 inline comments as done.

Address comments

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

This revision was automatically updated to reflect the committed changes.
bmahjour removed a subscriber: bmahjour.Feb 12 2020, 6:56 AM

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?
Ultimately, getting rid of whole-program-link is good, but is it really better to convert it to a list of passes here?

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?
This doesn't help the latter for BUILD_SHARED_LIBS=on because you still need to specify all libraries you have a dependence against.
My preference would be to have add_mlir_dialect generate a list of dialects which can be used to solve the second problem.

mehdi_amini marked an inline comment as done.Feb 21 2020, 9:02 AM
mehdi_amini added inline comments.
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!

mlir/lib/Dialect/StandardOps/DialectRegistration.cpp