This is an archive of the discontinued LLVM Phabricator instance.

[mlir][CAPI] Proposal: Always building a libMLIRPublicAPI.so.
ClosedPublic

Authored by stellaraccident on Nov 4 2020, 10:30 PM.

Details

Summary

We were discussing on discord regarding the need for extension-based systems like Python to dynamically link against MLIR (or else you can only have one extension that depends on it). Currently, when I set that up, I piggy-backed off of the flag that enables build libLLVM.so and libMLIR.so and depended on libMLIR.so from the python extension if shared library building was enabled. However, this is less than ideal.

In the current setup, libMLIR.so exports both all symbols from the C++ API and the C-API. The former is a kitchen sink and the latter is curated. We should be splitting them and for things that are properly factored to depend on the C-API, they should have the option to *only* depend on the C-API, and we should build that shared library no matter what. Its presence isn't just an optimization: it is a key part of the system.

To do this right, I needed to:

  • Introduce visibility macros into mlir-c/Support.h. These should work on both *nix and windows as-is.
  • Create a new libMLIRPublicAPI.so with just the mlir-c object files.
  • Compile the C-API with -fvisibility=hidden.
  • Conditionally depend on the libMLIR.so from libMLIRPublicAPI.so if building libMLIR.so (otherwise, also links against the static libs and will produce a mondo libMLIRPublicAPI.so).
  • Disable re-exporting of static library symbols that come in as transitive deps.

This gives us a dynamic linked C-API layer that is minimal and should work as-is on all platforms. Since we don't support libMLIR.so building on Windows yet (and it is not very DLL friendly), this will fall back to a mondo build of libMLIRPublicAPI.so, which has its uses (it is also the most size conscious way to go if you happen to know exactly what you need).

Sizes (release/stripped, Ubuntu 20.04):

Shared library build:
libMLIRPublicAPI.so: 121Kb
_mlir.cpython-38-x86_64-linux-gnu.so: 1.4Mb
mlir-capi-ir-test: 135Kb
libMLIR.so: 21Mb

Static build:
libMLIRPublicAPI.so: 5.5Mb (since this is a "static" build, this includes the MLIR implementation as non-exported code).
_mlir.cpython-38-x86_64-linux-gnu.so: 1.4Mb
mlir-capi-ir-test: 44Kb

Things like npcomp and circt which bring their own dialects/transforms/etc would still need the shared library build and code that links against libMLIR.so (since it is all C++ interop stuff), but hopefully things that only depend on the public C-API can just have the one narrow dep.

I spot checked everything with nm, and it looks good in terms of what is exporting/importing from each layer.

I'm not in a hurry to land this, but if it is controversial, I'll probably split off the Support.h and API visibility macro changes, since we should set that pattern regardless.

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Nov 4 2020, 10:30 PM

I'm fine with this, but at the same time I haven't wrapped my head around who benefits from this?
Which use-case would link to the libMLIRPublicAPI.so but couldn't link to libMLIR.so? Would it be a client that would have its own copy of MLIR *and* use libMLIRPublicAPI.so for another purpose? (because these two copies wouldn't be able to work together).

benvanik requested changes to this revision.Nov 5 2020, 1:05 PM

LGTM besides naming

One thing you may want to look into as a follow up (I've been putting off doing so) is whether you also want to specify the calling convention used (cdecl/stdcall/etc). That really depends on how you are using the library but it's possible in the 2020's on x64 hardware it's all the same and you don't need to do anything.

mlir/include/mlir-c/Support.h
27

the underline prefix is a bit odd here (no other use of underscore prefix macros I can find in LLVM/MLIR)
the convention in MLIR (and other projects) would be something like MLIR_FOO_EXPORT, so MLIR_CAPI_EXPORT here

This revision now requires changes to proceed.Nov 5 2020, 1:05 PM

I'm fine with this, but at the same time I haven't wrapped my head around who benefits from this?
Which use-case would link to the libMLIRPublicAPI.so but couldn't link to libMLIR.so? Would it be a client that would have its own copy of MLIR *and* use libMLIRPublicAPI.so for another purpose? (because these two copies wouldn't be able to work together).

As a matter of shared-library design, it is typically better, if you have gone to the trouble to define a well-constrained C-API, to have a way to use it that involves depending on something that *only* exposes the C-API, and aside from being a better defense against accidental linkage collisions, such a setup enables other scenarios that end up being used eventually, given the diversity in the deployment landscape:

  • The ability to build a mostly-static/hidden-visibility shared library that is the most size optimal possible by not having a backing C++ libMLIR.so at all. This will get used in various mobile and edge scenarios where it is important to retain some notion of dynamic linking while optimizing for size when you know exactly what needs to be included. As shown in the size tradeoff, out of the box this saves ~15MB of binary size with no further work (and there are more savings possible if configuring the project for hidden visibility and LTO). While not great for open-ecosystem things that might need to extend MLIR beyond what it was built for, a lot of use cases at this scale don't have that constraint and don't need to pay the tax.
  • Enables the construction of various "loader" shared libraries that can be used to shim multiple MLIR/LLVM instances into a single process, link-edit things for better standalone use, lookup the right backing implementation, or marshal calls across boundaries (ie. export to to a WebAssembly runtime or the like) -- basically all of the deployment tricks that are trivially easy to do with libraries that export a well defined C interface and are hard to do once you mix C++.
  • Provides an option to use the most principled path for DLL semantics on Windows.

I'm not saying all of the above come for free with this patch, but layering the shared library story in this way makes it much more tractable to do those things.

The ability to build a mostly-static/hidden-visibility shared library that is the most size optimal possible by not having a backing C++ libMLIR.so at all.

Yeah I remember shipping a mobile-oriented libLLVM that was manually providing the list of exported symbols to the linker for this.
And libLTO.so (which is used a linker plugin in for LTO in XCode for example) is still doing it this way: https://github.com/llvm/llvm-project/blob/master/llvm/tools/lto/lto.exports

Rebase, address comments, and adapt to python build changes.

stellaraccident marked an inline comment as done.Nov 5 2020, 3:56 PM

PTAL.

I haven't yet tried it, but this change should get us back to being able to build the python extensions on Windows (which broke with the change to multiple extensions). May need some more tweaking but in the right direction.

benvanik accepted this revision.Nov 5 2020, 4:09 PM
This revision is now accepted and ready to land.Nov 5 2020, 4:09 PM
mehdi_amini accepted this revision.Nov 5 2020, 5:52 PM
mehdi_amini added inline comments.
mlir/include/mlir-c/AffineExpr.h
184

There is no syntax for these annotation that would be a scope like the extern "C" { ... } ?

mlir/include/mlir-c/Support.h
60–61

Can you commit this ahead of time, so that regardless if we revert this patch this issue is fixed (I think it is independent right?)

mlir/lib/CAPI/CMakeLists.txt
36

I don't really know what this line is doing?

45

Nice expression! I didn't know this form.

mlir/lib/CAPI/Transforms/CMakeLists.txt
12

What about instead have a function:

add_mlir_public_c_api_library(...)

That would wrap around add_mlir_library and also do the work of mlir_capi_set_target_properties.

It could also potentially accumulate all the libraries that are added this way in a CMake global and replace the:

set(public_api_libs
  MLIRCAPIIR
  MLIRCAPIRegistration
  MLIRCAPIStandard
  MLIRCAPITransforms
)

in mlir/lib/CAPI/CMakeLists.txt

stellaraccident marked 4 inline comments as done.

Update to fix Windows buildbot.

mlir/include/mlir-c/AffineExpr.h
184

Not that I am aware of for C. There is some support for attributes on namespaces in C++ (not sure about declspecs, though).

stellaraccident marked an inline comment as done.

Comments and further fixes to Windows build.

Fix windows build.

PTAL - I fixed the windows build and verified that the python extensions build and function there (with dynamic linking!). There will be a couple of minor followups to fix some path issues to make it work for real but pretty close. With this change, the windows build bot does adequately test the dynamic linking/executing behavior and is authoritative.

mlir/lib/CAPI/CMakeLists.txt
36

Added comment: # Accumulate transitive deps of each exported lib into _DEPS.

mehdi_amini accepted this revision.Nov 6 2020, 8:30 AM

Still LGTM :)

This revision was landed with ongoing or failed builds.Nov 6 2020, 9:01 AM
This revision was automatically updated to reflect the committed changes.
ftynse added inline comments.Nov 6 2020, 9:59 AM
mlir/cmake/modules/AddMLIRPythonExtension.cmake
90 ↗(On Diff #303478)

This caused link error when using GNU ld.

: CommandLine Error: Option 'disable-symbolication' registered more than once!

when linking C API tests mlir-capi-ir-test and mlir-capi-pass-test.