Add support for toolchain compilation with LLVM_LINK_LLVM_DYLIB option

[fineg74]

This is to enable compilation of the toolchain with dynamic libraries

[v-klochkov]

If you add additional fix for this comment #6413 (comment) then the fix is good, IMO.
I would feel safer though if one more reviewer could take a look too, perhaps @bader or @steffenlarsen

[bader]

@fineg74, please, commit LLVM-SPIRV-Translator changes to https://github.com/KhronosGroup/SPIRV-LLVM-Translator/ repository. We regularly pull changes from Khronos repository here.

I thought that llvm-spirv tool build is fixed by KhronosGroup/SPIRV-LLVM-Translator#458. Has it been broken again?

[fineg74]

Created a PR there.
I believe KhronosGroup/SPIRV-LLVM-Translator#458 solved different problem. The problem this PR solves is a circular dependency that is created when shared libraries are built for toolchain build (building with static libraries do not has this problem as circular dependencies are allowed there).

[pvchupin]

It seems KhronosGroup/SPIRV-LLVM-Translator#1543 is the PR mentioned and it should land first.
@fineg74, please add detailed rationale in PR description there. Not sure if testing fails due to infrastructure temporary problems, you can restart if it was. I guess failed testing and no description are the reasons patch stuck for a while.

[bader]

KhronosGroup/SPIRV-LLVM-Translator#1543 is merged, so I expect no changes in llvm-spirv/* files.

I agree with Pavel that description should be improved. It's not clear why we need to change customize cmake files here. I expected add_llvm_library to handle linking with all intrinsics. I assume we are building all libraries from sources, so compilation and linking with intrinsics_gen should be no different than any other intrinsics like X86 or NVPTX intrinsics, which do not require this changes. Could you elaborate in the description the difference with regular intrinsics, which makes us customize cmake files for these tools, please?

[bader]

Please, remove duplication.
You can do it here by adding to the existing code.

if (LLVM_LINK_LLVM_DYLIB)
  list (REMOVE_ITEM LLVM_LINK_COMPONENTS SPIRVLib)
endif()

Another option: create a list w/o SPIRVLib and add it if LLVM_LINK_LLVM_DYLIB is not set.

On the other hand, I expect add_llvm_tool to handle LLVM_LINK_LLVM_DYLIB correctly. Could you clarify what is the problem with spirv-to-ir-wrapper tool and SPIRVLib library?

[fineg74]

The main issue is that llvm library depends on GenXIntrinsics and GenXIntrinsics depends on llvm which causes cmake to fail when shared library build option is used. The whole idea here is to break circular dependency cycle for shared library build by removing libraries from explicit linking list

[bader]

I expect GenXIntrinsics library to be linked statically to llvm.so library as all other libraries. Can we do that?

[fineg74]

GenXIntrinsics is built as static and therefore linked statically. The issue is that in current configuration there is a circular dependency between the libraries and while it is not a problem when building static llvm library, cmake throws an error when shared library llvm.so is built. Here is the error it produces:
CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle): "LLVMLTO" of type STATIC_LIBRARY depends on "LLVMPasses" (weak) depends on "LLVMSYCLLowerIR" (weak) depends on "LLVMGenXIntrinsics" (weak) depends on "LLVM" (weak) "LLVMOrcJIT" of type STATIC_LIBRARY depends on "LLVMPasses" (weak) depends on "LLVMSYCLLowerIR" (weak) depends on "LLVMGenXIntrinsics" (weak) depends on "LLVM" (weak) "LLVMPasses" of type STATIC_LIBRARY depends on "LLVMSYCLLowerIR" (weak) depends on "LLVMGenXIntrinsics" (weak) depends on "LLVM" (weak) "LLVMSYCLLowerIR" of type STATIC_LIBRARY depends on "LLVMGenXIntrinsics" (weak) depends on "LLVM" (weak) depends on "LLVMGenXIntrinsics" (strong) "LLVMGenXIntrinsics" of type STATIC_LIBRARY depends on "LLVM" (weak) "LLVM" of type SHARED_LIBRARY depends on "LLVMLTO" (weak) depends on "LLVMPasses" (weak) depends on "LLVMSYCLLowerIR" (weak) depends on "LLVMGenXIntrinsics" (weak) depends on "LLVMOrcJIT" (weak)

[bader]

Why did you remove GenXIntrinsics library from llvm.so library? I think this is the reason of cycle dependency. All libraries in the error message are part of the llvm.so, except LLVMGenXIntrinsics, which also depends on LLVM libraries. That's why we have a conflict: LLVMSYCLLowerIR library (part of llvm.so) depends on LLVMGenXIntrinsics (standalone), which depends on LLVMCore library (part of llvm.so).
If you make LLVMGenXIntrinsics as I suggested above, there should be no cyclic dependencies as there will be just one llvm.so library.

[fineg74]

I believe your suggestion is implemented : i.e. GenXIntrinsics is explicitly declared static if you try to build dynamic libraries and therefore it is linked statically with everything. The problem is that turning it into static is not enough to break the circular dependency as cmake is more sensitive to circular dependencies when dynamic libraries are built. That is why the dependency of GenXIntrinsics on llvm.so is replaced with dependencies on smaller libraries. We can discuss it in Teams if you believe it will be more productive.
What I did in the PR you referred above is I removed dependency of LLVMGenXIntrinsics on llvm.so and instead replaced it with dependencies on individual libraries. This is the way I broke circular dependency

[bader]

cmake is more sensitive to circular dependencies when dynamic libraries are built

I don't get what it means.
Shouldn't LLVMGenXIntrinsics be a part of LLVMTarget library or some other LLVM target library? Isn't the root case of the problem is that we put intrinsics in a separate library as opposite to how all other intrinsics are handled?

[fineg74]

When you build static libraries you are allowed to have circular dependencies. You are not allowed to have circular dependencies when you build dynamic libraries. That is why we don't see the issue during regular builds.
The root cause is as you mentioned earlier is that llvm.so depends on LLVMGenXIntrinsics.lib and LLVMGenXIntrinsics.lib depends on llvm.so.
It is possible to refactor everything and eliminate LLVMGenXIntrinsics.lib as a separate entity but I am offering less drastic solution

[kbobrovs]

Could you elaborate in the description the difference with regular intrinsics, which makes us customize cmake files for these tools, please?

GenX intrinsics come from an external repo during the build, unlike others, and they are built with the cmake files provided in that repo.

[bader]

I think sources are fetched during CMake configuration, so the build process can be same.

they are built with the cmake files provided in that repo

Thant's sounds like the real cause of CMake customizations on our side. I'm okay to keep changes in our repository for a while, but I suggest we open an issue to discuss with GenX instrinsics owners the way this repository is integrated with other LLVM projects. My main concern is that current approach with customizing CMake files for all projects linking with LLVM targets is not scalable. In my opinion, we can shoot for similar integration, where customization of one (or limited number) LLVM library is required, but rest of the projects should use standard CMake functions to integrate LLVM libraries.

[asudarsa]

Hi @fineg74

Is this an active PR? Please let me know if I needed to review this. Sorry for delay.

Thanks

[github-actions]

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

[github-actions]

This pull request was closed because it has been stalled for 30 days with no activity.