Building with -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON is broken

[fwyzard]

Building with -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON is broken as of 0d56408 :

cmake \
  -DCMAKE_BUILD_TYPE=Release \
  -DLLVM_TARGETS_TO_BUILD="X86;NVPTX" \
  -DLLVM_EXTERNAL_PROJECTS="llvm-spirv;sycl;opencl-aot" \
  -DLLVM_ENABLE_PROJECTS="clang;llvm-spirv;sycl;opencl-aot;libclc" \
  -DLLVM_EXTERNAL_SYCL_SOURCE_DIR=$SYCL_BASE/llvm/sycl \
  -DLLVM_EXTERNAL_LLVM_SPIRV_SOURCE_DIR=$SYCL_BASE/llvm/llvm-spirv \
  -DLLVM_BUILD_LLVM_DYLIB=ON \
  -DLLVM_LINK_LLVM_DYLIB=ON \
  -DLIBCLC_TARGETS_TO_BUILD="nvptx64--;nvptx64--nvidiacl" \
  -DSYCL_BUILD_PI_CUDA=ON \
  -DCUDA_TOOLKIT_ROOT_DIR=/usr/local/cuda \
  -DCUDA_USE_STATIC_CUDA_RUNTIME=OFF \
  $SYCL_BASE/llvm/llvm

make -j`nproc` sycl-toolchain

results in

...
[ 97%] Linking CXX executable ../../../../bin/llvm-spirv
CMakeFiles/llvm-spirv.dir/llvm-spirv.cpp.o: In function `convertSPIRV()::{lambda(std::ostream&)#1}::operator()(std::ostream&) const [clone .isra.169]':
llvm-spirv.cpp:(.text._ZZL12convertSPIRVvENKUlRSoE_clES_.isra.169+0x32): undefined reference to `SPIRV::convertSpirv(std::istream&, std::ostream&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, bool, bo
CMakeFiles/llvm-spirv.dir/llvm-spirv.cpp.o: In function `parseSpecConstOpt(llvm::StringRef, SPIRV::TranslatorOpts&)':
llvm-spirv.cpp:(.text._Z17parseSpecConstOptN4llvm9StringRefERN5SPIRV14TranslatorOptsE+0x16a): undefined reference to `llvm::getSpecConstInfo(std::istream&, std::vector<std::pair<unsigned int, unsigned int>, std::allocator<std::pair<unsig
CMakeFiles/llvm-spirv.dir/llvm-spirv.cpp.o: In function `main':
llvm-spirv.cpp:(.text.startup.main+0xc8e): undefined reference to `llvm::regularizeLlvmForSpirv(llvm::Module*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)'
llvm-spirv.cpp:(.text.startup.main+0xed6): undefined reference to `llvm::readSpirv(llvm::LLVMContext&, SPIRV::TranslatorOpts const&, std::istream&, llvm::Module*&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<c
llvm-spirv.cpp:(.text.startup.main+0x113e): undefined reference to `llvm::getSpecConstInfo(std::istream&, std::vector<std::pair<unsigned int, unsigned int>, std::allocator<std::pair<unsigned int, unsigned int> > >&)'
llvm-spirv.cpp:(.text.startup.main+0x13ba): undefined reference to `llvm::writeSpirv(llvm::Module*, SPIRV::TranslatorOpts const&, std::ostream&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)'
llvm-spirv.cpp:(.text.startup.main+0x15ef): undefined reference to `llvm::writeSpirv(llvm::Module*, SPIRV::TranslatorOpts const&, std::ostream&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)'
llvm-spirv.cpp:(.text.startup.main+0x170f): undefined reference to `SPIRV::SPIRVUseTextFormat'
collect2: error: ld returned 1 exit status
make[3]: *** [tools/llvm-spirv/tools/llvm-spirv/CMakeFiles/llvm-spirv.dir/build.make:85: bin/llvm-spirv] Error 1
make[2]: *** [CMakeFiles/Makefile2:70496: tools/llvm-spirv/tools/llvm-spirv/CMakeFiles/llvm-spirv.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:70865: tools/sycl/CMakeFiles/sycl-toolchain.dir/rule] Error 2
make: *** [Makefile:16657: sycl-toolchain] Error 2

[fwyzard]

This patch

diff --git a/llvm-spirv/tools/llvm-spirv/CMakeLists.txt b/llvm-spirv/tools/llvm-spirv/CMakeLists.txt
index 9aa96d9c41f..501c0daf4b6 100644
--- a/llvm-spirv/tools/llvm-spirv/CMakeLists.txt
+++ b/llvm-spirv/tools/llvm-spirv/CMakeLists.txt
@@ -14,7 +14,7 @@ add_llvm_tool(llvm-spirv
   NO_INSTALL_RPATH
 )
 
-if (LLVM_SPIRV_BUILD_EXTERNAL)
+if (LLVM_SPIRV_BUILD_EXTERNAL OR LLVM_LINK_LLVM_DYLIB)
   target_link_libraries(llvm-spirv PRIVATE LLVMSPIRVLib)
 endif()
 

seems to fix the build., though I don't know if that is the correct fix or just a workaround.

[bader]

@fwyzard, thanks for fixing this issue.
FYI, we don't use LLVM_BUILD_LLVM_DYLIB as it's not supported on Windows.
You patch looks okay to me, but I'd like Alexey Sotkin to approve before the merge.

[bader]

Out of curiosity: why do you build this configuration?
What are the advantages over BUILD_SHARED_LIBS?

[fwyzard]

In fact, we have been using BUILD_SHARED_LIBS so far, but while checking what it does I found this on Building a Distribution of LLVM:

General Distribution Guidance

...

Warning

One very important note: Distributions should never be built using the BUILD_SHARED_LIBS CMake option. That option exists for optimizing developer workflow only. Due to design and implementation decisions, LLVM relies on global data which can end up being duplicated across shared libraries resulting in bugs. As such this is not a safe way to distribute LLVM or LLVM-based tools.

...

Special Notes for Library-only Distributions

One of the most powerful features of LLVM is its library-first design mentality and the way you can compose a wide variety of tools using different portions of LLVM. Even in this situation using BUILD_SHARED_LIBS is not supported. If you want to distribute LLVM as a shared library for use in a tool, the recommended method is using LLVM_BUILD_LLVM_DYLIB, and you can use LLVM_DYLIB_COMPONENTS to configure which LLVM components are part of libLLVM.
Note: LLVM_BUILD_LLVM_DYLIB is not available on Windows.

...

Options for Reducing Size

...
The most impactful way to reduce binary size is to dynamically link LLVM into all the tools. This reduces code size by decreasing duplication of common code between the LLVM-based tools. This can be done by setting the following two CMake options to On: LLVM_BUILD_LLVM_DYLIB and LLVM_LINK_LLVM_DYLIB.

Warning

Distributions should never be built using the BUILD_SHARED_LIBS CMake option.

So now we are switching from BUILD_SHARED_LIBS to LLVM_LINK_LLVM_DYLIB.

[bader]

So you

want to distribute LLVM as a shared library for use in a tool

?

NOTE: we distribute our tools with LLVM libraries linked statically.
We might reduce the size of the tools if we link with dynamic LLVM library, but as noted in the LLVM documentation, it doesn't work on Windows. AFAIK, it's due to MS linker limitation on # of exported symbols - LLVM exports too many symbols.

[fwyzard]

Yes, that's the idea, though I haven't checked the final size of the installation recently.

[fwyzard]

Fixed by #1323, and upstream by KhronosGroup/SPIRV-LLVM-Translator#458 .