Page MenuHomePhabricator

[mlir] support building with BUILD_SHARED_LIBS=ON
Needs ReviewPublic

Authored by inouehrs on Jan 10 2020, 10:18 PM.

Details

Summary

BUILD_SHARED_LIBS is a LLVM's CMake option to build components as shared libraries (.so) instead of static library (.a). It is often used by developers to reduce build (especially linking) time as well as disk size.

This patch makes it possible to use BUILD_SHARED_LIBS=ON while compiling MLIR.
Currently, MLIR itself cannot be compiled as a shared library because of the cyclic dependency among MLIR components.
Hence this patch adds add_mlir_library cmake function, which sets the library type STATIC even BUILD_SHARED_LIBS is ON. Other than thar, add_mlir_library is equivalent with add_llvm_library.
By this, we can get most of the benefits of BUILD_SHARED_LIBS=ON since LLVM components are compiled as shared libraries while only MLIR, which is smaller than the entire LLVM, is still compiled as static libraries.

This patch should not affect if BUILD_SHARED_LIBS is set to OFF (default).

Diff Detail

Event Timeline

inouehrs created this revision.Jan 10 2020, 10:18 PM
inouehrs edited the summary of this revision. (Show Details)Jan 10 2020, 10:19 PM

Currently, MLIR itself cannot be compiled as a shared library because of the cyclic dependency among MLIR components.

Can we fix the cyclic dependency to begin with?

It seems like this patch is trying to dance around the underlying issue without addressing it.

mlir/cmake/modules/AddMLIR.cmake
2

Can you document this?

9

Can you add the *why* in the doc please.

65

According to the description, this code is identical to add_llvm_library but for the set(LIBTYPE STATIC), this is quite a non-negligible amount of code duplication. Can we refactor this? Can we call to add_llvm_library from here after setting the STATIC lib type?

Currently, MLIR itself cannot be compiled as a shared library because of the cyclic dependency among MLIR components.

Can we fix the cyclic dependency to begin with?

It seems like this patch is trying to dance around the underlying issue without addressing it.

Personally, I think it is cleaner to fix the shared library dependence, although I, too have taken the short route to enabling explicit STATIC libraries which can have circular dependencies.
I think that there is not a strong reason for the moment to have add_mlir_library, which is mostly a copy of the corresponding code from add_llvm_library.

inouehrs updated this revision to Diff 237694.Jan 13 2020, 8:37 AM
  • remove cyclic dependencies among MLIR components.
  • simplify add_mlir_library CMake function
inouehrs added a comment.EditedJan 13 2020, 8:44 AM

@mehdi_amini @stephenneuendorffer Thank you for the suggestions.

Can we fix the cyclic dependency to begin with?

I touched up dependencies and successfully removed cyclic dependency. However, the generated binary does not work well; it seems that module ops are not initialized properly if we compile MLIR as shared libraries.

According to the description, this code is identical to add_llvm_library but for the set(LIBTYPE STATIC), this is quite a non-negligible amount of code duplication. Can we refactor this? Can we call to add_llvm_library from here after setting the STATIC lib type?

I cleaned up the code in add_mlir_library. Now it is a simple wrapper around llvm_add_library (not add_llvm_library).

mlir/cmake/modules/AddMLIR.cmake
12

OK, so if I understand correctly, this means that LLVM can be built using shared libraries, but MLIR is built as static libraries unless explicitly specified in the CMakeFiles.txt. I think this is reasonably sane, and might be a decent stepping stone towards a better solution.

nicolasvasilache resigned from this revision.Jan 16 2020, 5:47 AM

Can you rebase the patch?

(It seems like you're not using arc, I can't even try to find your parent commit to apply it)

inouehrs updated this revision to Diff 238704.EditedJan 17 2020, 12:45 AM

Although I thought I removed all cyclic dependencies, I realize that there is still a cyclic dependency between MLIRAnalysis and MLIRPass. This seems to be a true cyclic dependency and I cannot remove this by only modifying CMake files. MLIRAnalysis is only one component that has been explicitly marked as STATIC and without this explicit STATIC option for MLIRAnalysis, the build failed during linking libMLIRAnalysis.so.

mehdi_amini added inline comments.Jan 18 2020, 6:33 PM
mlir/cmake/modules/AddMLIR.cmake
11

With this, this patch is not changing anything to building with -DBUILD_SHARED_LIBS=ON, shouldn't we remove this line?