Shared library support

A couple of weeks ago I raised the question on the tensorflow/mlir mailing list that I would like to see
support for building MLIR either with libLLVM.so or as it’s own libMLIR.so. Redirecting to Google Groups

There is a bigger question whether we should have a separate libMLIR or whether we should embed
the symbols into libLLVM.so.

Pros for the libMLIR.so approach:

  • MLIR right now is dependent but separate from LLVM proper
  • Reduces the size and the symbol count of libLLVM (the latter is already a problem)
  • Allows consumers of builds from LLVM to choose if they want to load MLIR

Cons for the libMLIR.so approach:

  • May make it harder for MLIR to become used in LLVM proper (circular dependency)

Paul referenced https://reviews.llvm.org/D61909 which is for libclang-cpp.so and has some notes on the design thinking there and how one might refactor llvm-shlib.

This created several Phabricator PRs:

(breaking apart because of discourse user limitations)

I’ve broken the circular dependencies:
https://reviews.llvm.org/D73654
https://reviews.llvm.org/D73655

The combination of these patches should allow BUILD_SHARED_LIBS=on.

1 Like

Another wrinkle will be:

In D72822 @rridle wrote:

You are going to run into a much bigger problem given that this depends on ClassOf, which is a static typeid that is used pervasively across the entire code base. Making that work with shared libraries is likely the most important(difficult?) task item.

I must admit that my understanding of C++ is not strong enough to offer any salient thoughts on that.

Sorry, there is a typo in that quote. It should be ClassID.

That class will have to be modified to work correctly in the presence of shared libraries. For Linux I think it should mostly just work as-is(% maybe tweaking the visibility). For Windows DLL, I think we will have to fallback to comparing class names(using llvm::getTypeName?). It would help if we can start clarifying which problems are affecting which systems. This makes it easier as some systems, mainly MSVC, have no problems that can be worked around. I’m more than happy to fix this myself, but it would help to have some repro instructions that can be used for testing the various scenarios.

I plan to create a isolated reproducer next week :slight_smile: right now encountered while experimenting with MLIR in JuliaLang and that setup is convoluted enough that it isn’t a reasonable reproducer.

OK, the patches have landed to allow BUILD_SHARED_LIBS=on to work… Thanks for the help and suggestions.

2 Likes

I’ve been starting to work with @vchuravy 's patch here https://reviews.llvm.org/D72554
With some tweaks, I was able to get this close to working. I think the next major problem is dealing with the structure of the CMakefiles. When building with LLVM_LINK_LLVM_DYLIB, llvm_add_library() rewrites the LINK_LIBS keyword. This means that the current style using target_link_library doesn’t work, instead we have to switch to using the LINK_LIBS keyword to get the proper dependencies.

Similarly, llvm_add_library can also generate new targets, so it needs to know about the dependencies of those targets. The solution is to move the current style with add_dependencies() into the DEPEND keyword.

I think I’m close to getting something working using this.

1 Like

I’m curious what people think about the test dialect dependencies for mlir-opt. Currently, these are somewhat hard dependencies, being referenced by mlir-opt directly. However, I chose not to include these in libMLIR.so. Are there strong feelings that these should exist in libMLIR.so? One thought I had was to build a ‘test’ version of mlir-opt which includes the test dialects and is used to run the tests for the test dialects.

I don’t think the test dialect should be in libMLIR.so, but what is the problem with having them linked to mlir-opt?

In general the tools/ binaries are for debug/test/development purpose, so having the test dialect there shouldn’t hurt?

There’s no real problem per se. I guess I think of mlir-opt as a little more of a production binary, but in practice we might assume that everyone is going to add their own dialect and so mlir-opt is necessarily incomplete. I guess this was part of my thinking about having a test version of mlir-opt, since it would demonstrate “here’s how to add something (namely the test dialects) to mlir-opt”.

For IREE I forked mlir-opt.cpp to get rid not only of the test dialects, but also of some further dialects getting registered by registerAllDialects(). For the static build, it was required to link those in. And yes, we need to add the IREE specific dialects. So I agree that the test dialects should not be in libMLIR.so, but I have no strong opinion about mlir-opt itself.

Curiosity: why not put the test dialect into libMLIR.so? It seems weird that it is not in the lib/Dialect directory and treated like everything else.

In my view, libMLIR is never going to be a close fit for any given application. Anyone who cares about code size will have to build their own thing.

libMLIR might not be a great fit for any given application, but it should be good enough for most things. I think requiring (or even encouraging) people to roll their own is a bad idea. I think getting to the point where most people can take libMLIR and add a new dialect or two and get a working tool will be a big step forward and lower the barrier to entry here. It’s already hard enough for people to get into writing code that does something useful. MLIR is still a ways away from the llvm model of “write a pass, dynamically load it into opt and show something”, which allows many people to be quickly successful.

All that said: moving the test dialects into libMLIR.so is simple if that’s what people want. :slight_smile:

1 Like

I see the relationship between test dialects and the other dialect similarly to the C++ unit-tests and the other libraries: we don’t ship our unit-tests, their sole-purpose is to make sure that we can have test coverage for a bunch of things internally in the project. It isn’t clear to me why we would not keep all this isolated under the test directory.

I can see it both ways for sure. To me, the current situation is just redundant because there are dialects that don’t live in the same place as everything else.

I think your point about “not wanting to support” the test dialect is sound, but there is no stability guarantee for any dialects (as far as I’m aware), and many of them are experimental. There are still those of us who want to completely reevaluate the standard dialect someday :slight_smile:

I guess this is just a way of saying that “we have something weird, and I don’t see the value of it”. It seems simpler to manage test dialect the same as all the rest.