Page MenuHomePhabricator

[MLIR] Fixes for shared library dependencies.
ClosedPublic

Authored by stephenneuendorffer on Jan 29 2020, 11:37 AM.

Details

Summary

This patch is a step towards enabling BUILD_SHARED_LIBS=on, which
builds most libraries as DLLs instead of statically linked libraries.
The main effect of this is that incremental build times are greatly
reduced, since usually only one library need be relinked in response
to isolated code changes.

The bulk of this patch is fixing incorrect usage of cmake, where library
dependencies are listed under add_dependencies rather than under
target_link_libraries or under the LINK_LIBS tag. Correct usage should be
like this:

add_dependencies(MLIRfoo MLIRfooIncGen)
target_link_libraries(MLIRfoo MLIRlib1 MLIRlib2)

A separate issue is that in cmake, dependencies between static libraries
are automatically included in dependencies. In the above example, if MLIBlib1
depends on MLIRlib2, then it is sufficient to have only MLIRlib1 in the
target_link_libraries. When compiling with shared libraries, it is necessary
to have both MLIRlib1 and MLIRlib2 specified if MLIRfoo uses symbols from both.

Diff Detail

Event Timeline

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

A separate issue is that in cmake, dependencies between static libraries
are automatically included in dependencies. In the above example, if MLIBlib1
depends on MLIRlib2, then it is sufficient to have only MLIRlib1 in the
target_link_libraries. When compiling with shared libraries, it is necessary
to have both MLIRlib1 and MLIRlib2 specified.

I believe this is only an issue if the target is directly using symbols from MLIRlib2 though.
If it only needs symbols from MLIRLib1 then this shouldn't be an issue right?

mehdi_amini accepted this revision.Jan 29 2020, 9:53 PM
mehdi_amini added inline comments.
mlir/lib/Dialect/FxpMathOps/CMakeLists.txt
9

It seems we've been using target_link_libraries elsewhere but they are the same, can we align on one form for consistency?

This revision is now accepted and ready to land.Jan 29 2020, 9:53 PM
nicolasvasilache accepted this revision.Feb 1 2020, 10:14 AM

Thank you Stephen!

A separate issue is that in cmake, dependencies between static libraries
are automatically included in dependencies. In the above example, if MLIBlib1
depends on MLIRlib2, then it is sufficient to have only MLIRlib1 in the
target_link_libraries. When compiling with shared libraries, it is necessary
to have both MLIRlib1 and MLIRlib2 specified.

I believe this is only an issue if the target is directly using symbols from MLIRlib2 though.
If it only needs symbols from MLIRLib1 then this shouldn't be an issue right?

Yes, I agree. I've edited the comment to be clearer in this regard.

stephenneuendorffer edited the summary of this revision. (Show Details)

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

stephenneuendorffer marked an inline comment as done.Feb 3 2020, 10:31 PM
stephenneuendorffer added inline comments.
mlir/lib/Dialect/FxpMathOps/CMakeLists.txt
9

I fixed these to use target_link_libraries instead of LINK_LIBS

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This revision was automatically updated to reflect the committed changes.

This commit causes errors when linking mlir-opt (I'm not using shared libraries):

[...]
/w/bld/org/lib/libMLIRGPUtoCUDATransforms.a(ConvertKernelFuncToCubin.cpp.o): In function `(anonymous namespace)::GpuKernelToCubinPass::runOnOperation()':
ConvertKernelFuncToCubin.cpp:(.text._ZN12_GLOBAL__N_120GpuKernelToCubinPass14runOnOperationEv+0x44): undefined reference to `LLVMInitializeNVPTXTarget'
ConvertKernelFuncToCubin.cpp:(.text._ZN12_GLOBAL__N_120GpuKernelToCubinPass14runOnOperationEv+0x49): undefined reference to `LLVMInitializeNVPTXTargetInfo'
ConvertKernelFuncToCubin.cpp:(.text._ZN12_GLOBAL__N_120GpuKernelToCubinPass14runOnOperationEv+0x4e): undefined reference to `LLVMInitializeNVPTXTargetMC'
ConvertKernelFuncToCubin.cpp:(.text._ZN12_GLOBAL__N_120GpuKernelToCubinPass14runOnOperationEv+0x53): undefined reference to `LLVMInitializeNVPTXAsmPrinter'
ConvertKernelFuncToCubin.cpp:(.text._ZN12_GLOBAL__N_120GpuKernelToCubinPass14runOnOperationEv+0x60): undefined reference to `mlir::translateModuleToNVVMIR(mlir::Operation*)'
ConvertKernelFuncToCubin.cpp:(.text._ZN12_GLOBAL__N_120GpuKernelToCubinPass14runOnOperationEv+0x2d0): undefined reference to `llvm::MCTargetOptions::MCTargetOptions()'
clang-9: error: linker command failed with exit code 1 (use -v to see invocation)
tools/mlir/tools/mlir-opt/CMakeFiles/mlir-opt.dir/build.make:114: recipe for target 'bin/mlir-opt' failed
make[2]: *** [bin/mlir-opt] Error 1
make[2]: Target 'tools/mlir/tools/mlir-opt/CMakeFiles/mlir-opt.dir/build' not remade because of errors.
[...]