Remove libMLIRPublicAPI DSO.

[stellaraccident]

Summary

libMLIRPublicAPI.so came into existence early when the Python and C-API were being co-developed because the Python extensions need a single DSO which exports the C-API to link against. It really should never have been exported as a mondo library in the first place, which has caused no end of problems in different linking modes, etc (i.e. the CAPI tests depended on it).

This patch does a mechanical move that:

• Makes the C-API tests link directly to their respective libraries.
• Creates a libMLIRPythonCAPI as part of the Python bindings which assemble to exact DSO that they need.

This has the effect that the C-API is no longer monolithic and can be subset and used piecemeal in a modular fashion, which is necessary for downstreams to only pay for what they use. There are additional, more fundamental changes planned for how the Python API is assembled which should make it more out of tree friendly, but this minimal first step is necessary to break the fragile dependency between the C-API and Python API.

Downstream actions required:

• If using the C-API and linking against MLIRPublicAPI, you must instead link against its constituent components. As a reference, the Python API dependencies are in lib/Bindings/Python/CMakeLists.txt and approximate the full set of dependencies available.
• If you have a Python API project that was previously linking against MLIRPublicAPI (i.e. to add its own C-API DSO), you will want to s/MLIRPublicAPI/MLIRPythonCAPI/ and all should be as it was. There are larger changes coming in this area but this part is incremental.

[stellaraccident]

Fix Windows build.

[mehdi_amini]

I'm not sure I understand the problem you're trying to fix here: having a single DSO exporting all of the C API seems valuable to me.

And I didn't quite get the problem with C unit-test linking to it? Is it because the C unit-test would embed another copy of libSupport from libMLIRPublicApi.so ?

It may very well be that libMLIRPublicApi.so should only link libMLIR.so and no other static archive than the C API bindings. Clients of libMLIRPublicApi.so that require any C++ code should get it from libMLIR.so as well.

The complexity is always when you want more fine grain control over the amount of code to include: people building specific compilers using MLIR are likely to at some point customize their own build and not use libMLIRPublicApi.so / libMLIR.so, but as a starting point and as a point of inter-op between projects it seems like a good setup?

mlir/cmake/modules/AddMLIR.cmake

186

I'm a bit puzzled bit this method now: what is the public_c_api_library remaining? Aren't you removing it?

What is the intended use the MLIR_PUBLIC_C_API_LIBS property now?

mlir/lib/Bindings/Python/CMakeLists.txt

52

Carrying back my comment from the Mingw "fix" last week: shouldn't ${_DEPS} be just libMLIR.so when it is used?

[stellaraccident]

In D106369#2890881, @mehdi_amini wrote:

I'm not sure I understand the problem you're trying to fix here: having a single DSO exporting all of the C API seems valuable to me.

And I didn't quite get the problem with C unit-test linking to it? Is it because the C unit-test would embed another copy of libSupport from libMLIRPublicApi.so ?

There has been a running firefight in downstreams (i.e. circt is carrying a local patch that effectively neuters this) with respect to this. Having this kind of bottleneck where we force dynamic linking just because you want to use the C-API was a bad design and doesn't cover how we actually want to use it (i.e. statically link). It turns out that this whole setup is super unstable across platforms, and it turns out that everyone who is using it is feeling pain. Better to remove it and layer this like everything else instead of having it be its own weird thing.

It may very well be that libMLIRPublicApi.so should only link libMLIR.so and no other static archive than the C API bindings. Clients of libMLIRPublicApi.so that require any C++ code should get it from libMLIR.so as well.

I don't think we can say that. That may be a way that people want to use it, but it is very much not want we want to be doing for Python, which is the reason I created this DSO to begin with.

The complexity is always when you want more fine grain control over the amount of code to include: people building specific compilers using MLIR are likely to at some point customize their own build and not use libMLIRPublicApi.so / libMLIR.so, but as a starting point and as a point of inter-op between projects it seems like a good setup?

It hasn't worked out that way. I would be ok having a *normal* library named MLIRPublicAPI that folks can use as a convenience (ie. a static lib for static builds and a shared lib for shared builds) but we shouldn't be introducing a hard dependency on a DSO in this way.

This all might be easier to chat about f2f. I'll ping you.

mlir/cmake/modules/AddMLIR.cmake

186

Right - forgot to remove the property. I think this still has some value with respect to the visibility support. I'm planning to generalize that a bit in a followup, which may obviate the need for this function -- just not there yet.

mlir/lib/Bindings/Python/CMakeLists.txt

52

Carrying back my comment from the Mingw "fix" last week: shouldn't ${_DEPS} be just libMLIR.so when it is used?

Not for the Python API. We don't (and will not) use libMLIR for this use case. In the common deployment case, this is a statically linked DSO, per best practices for Python deployment.

[stellaraccident]

Fix windows build better.

[stellaraccident]

Remove global property of CAPI libs.

[stellaraccident]

Thanks for the review and conversation, Mehdi. Summarizing:

• We need to be switching to a mode where the Python APIs compose for building Python based systems, and those need to be based on static linkage in the way starting to be set down here.
• The MLIR C-API tests depending on fine-grained libs helps keep things tidy and, since they are written in C, there is already an implicit barrier keeping them depending purely on the C-API (which is the layering we want).
• Nothing in this patch is opposed to having a single "Public C-API" library entry point for default use in the future. It is just that with the above two, the only in-tree (and known required) uses of libMLIRPublicAPI.so are now gone, and it is better to remove things that lack a reason for existing. It can be added back later if useful, and at that point, it should just be a normal library (not a forced shared-library).

[stellaraccident]

Fix windows visibility (verified locally).

[stellaraccident]

Fix python API on windows (some pre-existing test failures but generally works).

mlir/cmake/modules/AddMLIR.cmake

add_llvm_install_targets(install-${name}
                             DEPENDS ${name}
                             COMPONENT ${name})
   endif()
   set_property(GLOBAL APPEND PROPERTY MLIR_ALL_LIBS ${name})
   endif()
   set_property(GLOBAL APPEND PROPERTY MLIR_EXPORTS ${name})
 endfunction()
 
-# Declare an mlir library which is part of the public C-API and will be
-# compiled and exported into libMLIRPublicAPI.so/MLIRPublicAPI.dll.
-# This shared library is built regardless of the overall setting of building
-# libMLIR.so (which exports the C++ implementation).
+# Declare an mlir library which is part of the public C-API.
 function(add_mlir_public_c_api_library name)
   add_mlir_library(${name}
     ${ARGN}
     # NOTE: Generates obj.${name} which is used for shared library building.
     OBJECT
     EXCLUDE_FROM_LIBMLIR
     ADDITIONAL_HEADER_DIRS
     ${MLIR_MAIN_INCLUDE_DIR}/mlir-c
   )
   # API libraries compile with hidden visibility and macros that enable
   # exporting from the DLL. Only apply to the obj lib, which only affects
   # the exports via a shared library.
   set_target_properties(obj.${name}
     PROPERTIES
     CXX_VISIBILITY_PRESET hidden
   )
   target_compile_definitions(obj.${name}
     PRIVATE
     -DMLIR_CAPI_BUILDING_LIBRARY=1
   )
   set_property(GLOBAL APPEND PROPERTY MLIR_PUBLIC_C_API_LIBS ${name})
 endfunction()
 
 # Declare the library associated with a dialect.
 function(add_mlir_dialect_library name)
   set_property(GLOBAL APPEND PROPERTY MLIR_DIALECT_LIBS ${name})
   add_mlir_library(${ARGV} DEPENDS mlir-headers)
 endfunction(add_mlir_dialect_library)
 

mlir/cmake/modules/AddMLIRPython.cmake

function(add_mlir_python_extension libname extname)
   else()
     set(PYEXT_LINK_MODE SHARED)
     set(PYEXT_LIBADD ${Python3_LIBRARIES})
   endif()
 
   # The actual extension library produces a shared-object or DLL and has
   # sources that must be compiled in accordance with pybind11 needs (RTTI and
   # exceptions).
-  add_library(${libname} ${PYEXT_LINK_MODE}
+  add_library(${libname}
+    ${PYEXT_LINK_MODE}
     ${ARG_SOURCES}
   )
 
   target_include_directories(${libname} PRIVATE
     "${Python3_INCLUDE_DIRS}"
     "${pybind11_INCLUDE_DIR}"
   )
 
function(add_mlir_python_extension libname extname)
   # script that explicitly hides anything but the PyInit_* symbols, allowing gc
   # to take place.
   set_target_properties(${libname} PROPERTIES CXX_VISIBILITY_PRESET "hidden")
 
   # Python extensions depends *only* on the public API and LLVMSupport unless
   # if further dependencies are added explicitly.
   target_link_libraries(${libname}
     PRIVATE
-    MLIRPublicAPI
-    LLVMSupport
     ${ARG_LINK_LIBS}
     ${PYEXT_LIBADD}
   )
 
   target_link_options(${libname}
     PRIVATE
       # On Linux, disable re-export of any static linked libraries that
       # came through.

mlir/include/mlir-c/Support.h

 
 #include <stdbool.h>
 #include <stddef.h>
 #include <stdint.h>
 
 //===----------------------------------------------------------------------===//
 // Visibility annotations.
 // Use MLIR_CAPI_EXPORTED for exported functions.
+//
+// On Windows, if MLIR_CAPI_ENABLE_WINDOWS_DLL_DECLSPEC is defined, then
+// __declspec(dllexport) and __declspec(dllimport) will be generated. This
+// can only be enabled if actually building DLLs. It is generally, mutually
+// exclusive with the use of other mechanisms for managing imports/exports
+// (i.e. CMake's WINDOWS_EXPORT_ALL_SYMBOLS feature).
 //===----------------------------------------------------------------------===//
 
-#if defined(MLIR_CAPI_DISABLE_VISIBILITY_ANNOTATIONS)
+#if (defined(_WIN32) || defined(__CYGWIN__)) &&                                \
+    defined(MLIR_CAPI_ENABLE_WINDOWS_DLL_DECLSPEC)
+// Visibility annotations disabled.
 #define MLIR_CAPI_EXPORTED
 #elif defined(_WIN32) || defined(__CYGWIN__)
 // Windows visibility declarations.
 #if MLIR_CAPI_BUILDING_LIBRARY
 #define MLIR_CAPI_EXPORTED __declspec(dllexport)
 #else
 #define MLIR_CAPI_EXPORTED __declspec(dllimport)
 #endif

mlir/lib/Bindings/Python/CMakeLists.txt

 include(AddMLIRPython)
 add_custom_target(MLIRBindingsPythonExtension)
 
 ################################################################################
+# All python extensions must link through one DSO which exports the CAPI, and
+# this must have a globally unique name amongst all embeddors of the python
+# library since it will effectively have global scope.
+#
+# The presence of this aggregate library is part of the long term plan, but its
+# use needs to be made more flexible.
+################################################################################
+
+set(public_api_libs
+  MLIRCAPIConversion
+  MLIRCAPIDebug
+  MLIRCEXECUTIONENGINE
+  MLIRCAPIIR
+  MLIRCAPIRegistration
+  MLIRCAPITransforms
+
+  # Dialects
+  MLIRCAPIAsync
+  MLIRCAPIGPU
+  MLIRCAPILinalg
+  MLIRCAPILLVM
+  MLIRCAPIShape
+  MLIRCAPISparseTensor
+  MLIRCAPIStandard
+  MLIRCAPISCF
+  MLIRCAPITensor
+)
+
+foreach(lib ${public_api_libs})
+  if(XCODE)
+    # Xcode doesn't support object libraries, so we have to trick it into
+    # linking the static libraries instead.
+    list(APPEND _DEPS "-force_load" ${lib})
+  else()
+    list(APPEND _OBJECTS $<TARGET_OBJECTS:obj.${lib}>)
+  endif()
+  # Accumulate transitive deps of each exported lib into _DEPS.
+  list(APPEND _DEPS  $<TARGET_PROPERTY:${lib},LINK_LIBRARIES>)
+endforeach()
+
+add_mlir_library(MLIRPythonCAPI
+  PARTIAL_SOURCES_INTENDED
+  SHARED
+  ${_OBJECTS}
+  EXCLUDE_FROM_LIBMLIR
+  LINK_LIBS
+  ${_DEPS}
+)
+if(MSVC)
+  set_property(TARGET MLIRPythonCAPI PROPERTY WINDOWS_EXPORT_ALL_SYMBOLS ON)
+endif()
+
+################################################################################
 # Build core python extension
 ################################################################################
 add_mlir_python_extension(MLIRCoreBindingsPythonExtension _mlir
   INSTALL_DIR
     python
   SOURCES
     DialectLinalg.cpp
     DialectSparseTensor.cpp
     MainModule.cpp
     IRAffine.cpp
     IRAttributes.cpp
     IRCore.cpp
     IRModule.cpp
     IRTypes.cpp
     PybindUtils.cpp
     Pass.cpp
     ExecutionEngine.cpp
+  LINK_LIBS PRIVATE
+    MLIRPythonCAPI
 )
 add_dependencies(MLIRBindingsPythonExtension MLIRCoreBindingsPythonExtension)
 
 add_subdirectory(Transforms)
 add_subdirectory(Conversions)
 
 add_mlir_python_extension(MLIRAllPassesRegistrationBindingsPythonExtension _mlirAllPassesRegistration
   INSTALL_DIR
     python
   SOURCES
     AllPassesRegistration.cpp
+  LINK_LIBS PRIVATE
+    MLIRPythonCAPI
 )
 add_dependencies(MLIRBindingsPythonExtension MLIRAllPassesRegistrationBindingsPythonExtension)
 
 add_mlir_python_extension(MLIRAsyncPassesBindingsPythonExtension _mlirAsyncPasses
   INSTALL_DIR
     python
   SOURCES
     AsyncPasses.cpp
+  LINK_LIBS PRIVATE
+    MLIRPythonCAPI
 )
 add_dependencies(MLIRBindingsPythonExtension MLIRAsyncPassesBindingsPythonExtension)
 
 add_mlir_python_extension(MLIRSparseTensorPassesBindingsPythonExtension _mlirSparseTensorPasses
   INSTALL_DIR
     python
   SOURCES
     SparseTensorPasses.cpp
+  LINK_LIBS PRIVATE
+    MLIRPythonCAPI
 )
 add_dependencies(MLIRBindingsPythonExtension MLIRSparseTensorPassesBindingsPythonExtension)
 
 add_mlir_python_extension(MLIRGPUPassesBindingsPythonExtension _mlirGPUPasses
   INSTALL_DIR
     python
   SOURCES
     GPUPasses.cpp
+  LINK_LIBS PRIVATE
+    MLIRPythonCAPI
 )
 add_dependencies(MLIRBindingsPythonExtension MLIRGPUPassesBindingsPythonExtension)
 
 add_mlir_python_extension(MLIRLinalgPassesBindingsPythonExtension _mlirLinalgPasses
   INSTALL_DIR
     python
   SOURCES
     LinalgPasses.cpp
+  LINK_LIBS PRIVATE
+    MLIRPythonCAPI
 )
 add_dependencies(MLIRBindingsPythonExtension MLIRLinalgPassesBindingsPythonExtension)

mlir/lib/Bindings/Python/Conversions/CMakeLists.txt

 ################################################################################
 # Build python extension
 ################################################################################
 
 add_mlir_python_extension(MLIRConversionsBindingsPythonExtension _mlirConversions
   INSTALL_DIR
     python
   SOURCES
   Conversions.cpp
+  LINK_LIBS PRIVATE
+    MLIRPythonCAPI
 )

mlir/lib/Bindings/Python/Transforms/CMakeLists.txt

 ################################################################################
 # Build python extension
 ################################################################################
 
 add_mlir_python_extension(MLIRTransformsBindingsPythonExtension _mlirTransforms
   INSTALL_DIR
     python
   SOURCES
   Transforms.cpp
-)
+  LINK_LIBS PRIVATE
+    MLIRPythonCAPI
+)

mlir/lib/CAPI/CMakeLists.txt

 add_subdirectory(Debug)
 add_subdirectory(Dialect)
 add_subdirectory(Conversion)
 add_subdirectory(ExecutionEngine)
 add_subdirectory(IR)
 add_subdirectory(Registration)
 add_subdirectory(Transforms)
-
-
-################################################################################
-# libMLIRPublicAPI shared library/DLL.
-################################################################################
-
-get_property(public_api_libs GLOBAL PROPERTY MLIR_PUBLIC_C_API_LIBS)
-
-foreach(lib ${public_api_libs})
-  if(XCODE)
-    # Xcode doesn't support object libraries, so we have to trick it into
-    # linking the static libraries instead.
-    list(APPEND _DEPS "-force_load" ${lib})
-  else()
-    list(APPEND _OBJECTS $<TARGET_OBJECTS:obj.${lib}>)
-  endif()
-  # Accumulate transitive deps of each exported lib into _DEPS.
-  list(APPEND _DEPS  $<TARGET_PROPERTY:${lib},LINK_LIBRARIES>)
-endforeach()
-
-if(MINGW)
-    set(MLIR_LINK_MLIR_DYLIB 0)
-else()
-    set(MLIR_LINK_MLIR_DYLIB ${LLVM_BUILD_LLVM_DYLIB})
-endif()
-
-add_mlir_library(MLIRPublicAPI
-  SHARED
-  ${_OBJECTS}
-  EXCLUDE_FROM_LIBMLIR
-  LINK_LIBS
-  # Dependency on the implementation shared library.
-  $<$<BOOL:${MLIR_LINK_MLIR_DYLIB}>:MLIR>
-  ${_DEPS}
-)
-
-target_link_options(
-  MLIRPublicAPI
-  PRIVATE
-    # On Linux, disable re-export of any static linked libraries that
-    # came through.
-    $<$<PLATFORM_ID:Linux>:LINKER:--exclude-libs,ALL>
-)

mlir/python/mlir/_cext_loader.py

except ModuleNotFoundError:
   def _load_extension(name):
     import importlib
     return importlib.import_module(name)  # i.e. '_mlir' at the top level
 else:
   # Packaged distribution.
   _load_extension = _mlir_libs.load_extension
   _preload_dependency = _mlir_libs.preload_dependency
 
-_preload_dependency("MLIRPublicAPI")
+_preload_dependency("MLIRPythonCAPI")
 
 # Expose the corresponding C-Extension module with a well-known name at this
 # top-level module. This allows relative imports like the following to
 # function:
 #   from .._cext_loader import _cext
 # This reduces coupling, allowing embedding of the python sources into another
 # project that can just vary based on this top-level loader module.
 _cext = _load_extension("_mlir")

mlir/test/CAPI/CMakeLists.txt

 function(_add_capi_test_executable name)
   cmake_parse_arguments(ARG
     ""
     ""
-    ""
+    "LINK_LIBS"
     ${ARGN})
   set(LLVM_LINK_COMPONENTS
     )
   add_llvm_executable(${name}
     PARTIAL_SOURCES_INTENDED
     ${ARG_UNPARSED_ARGUMENTS})
   llvm_update_compile_flags(${name})
   target_link_libraries(${name}
-    PRIVATE
-    MLIRPublicAPI)
+    ${ARG_LINK_LIBS})
 endfunction(_add_capi_test_executable)
 
 _add_capi_test_executable(mlir-capi-execution-engine-test
   execution_engine.c
-DEPENDS
-  MLIRConversionPassIncGen
+LINK_LIBS PRIVATE
+  MLIRCAPIConversion
+  MLIRCEXECUTIONENGINE
+  MLIRCAPIRegistration
 )
 
 _add_capi_test_executable(mlir-capi-ir-test
   ir.c
+  LINK_LIBS PRIVATE
+    MLIRCAPIIR
+    MLIRCAPIStandard
+    MLIRCAPIRegistration
 )
 
 _add_capi_test_executable(mlir-capi-llvm-test
   llvm.c
+  LINK_LIBS PRIVATE
+    MLIRCAPIIR
+    MLIRCAPILLVM
+    MLIRCAPIRegistration
 )
 
 _add_capi_test_executable(mlir-capi-pass-test
   pass.c
+  LINK_LIBS PRIVATE
+    MLIRCAPIIR
+    MLIRCAPIRegistration
+    MLIRCAPITransforms
 )
 
 _add_capi_test_executable(mlir-capi-sparse-tensor-test
   sparse_tensor.c
+  LINK_LIBS PRIVATE
+    MLIRCAPIIR
+    MLIRCAPIRegistration
+    MLIRCAPISparseTensor
 )