[all-commits] [llvm/llvm-project] 80fe2f: [mlir][CAPI] Proposal: Always building a libMLIRPu...

[Stella Laurenzo via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: 80fe2f61fac9f72825f907637a9c63aca32d8f14
      https://github.com/llvm/llvm-project/commit/80fe2f61fac9f72825f907637a9c63aca32d8f14
  Author: Stella Laurenzo <stellaraccident at gmail.com>
  Date:   2020-11-06 (Fri, 06 Nov 2020)

  Changed paths:
    M mlir/cmake/modules/AddMLIR.cmake
    M mlir/cmake/modules/AddMLIRPythonExtension.cmake
    M mlir/include/mlir-c/AffineExpr.h
    M mlir/include/mlir-c/AffineMap.h
    M mlir/include/mlir-c/Diagnostics.h
    M mlir/include/mlir-c/IR.h
    M mlir/include/mlir-c/Pass.h
    M mlir/include/mlir-c/Registration.h
    M mlir/include/mlir-c/StandardAttributes.h
    M mlir/include/mlir-c/StandardDialect.h
    M mlir/include/mlir-c/StandardTypes.h
    M mlir/include/mlir-c/Support.h
    M mlir/include/mlir-c/Transforms.h
    M mlir/lib/Bindings/Python/CMakeLists.txt
    M mlir/lib/CAPI/CMakeLists.txt
    M mlir/lib/CAPI/IR/CMakeLists.txt
    M mlir/lib/CAPI/Registration/CMakeLists.txt
    M mlir/lib/CAPI/Standard/CMakeLists.txt
    M mlir/lib/CAPI/Transforms/CMakeLists.txt
    M mlir/lib/CAPI/Transforms/Passes.cpp
    M mlir/test/CAPI/CMakeLists.txt
    M mlir/tools/mlir-tblgen/PassCAPIGen.cpp

  Log Message:
  -----------
  [mlir][CAPI] Proposal: Always building a libMLIRPublicAPI.so.

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.

Reviewed By: mehdi_amini, benvanik

Differential Revision: https://reviews.llvm.org/D90824