Skip to content

Add support for toolchain compilation with LLVM_LINK_LLVM_DYLIB option #6413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Add support for toolchain compilation with LLVM_LINK_LLVM_DYLIB option #6413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e2fd946
Add support for dynamic LLVM build
fineg74 Jul 5, 2022
2c604d5
Fix a build break
fineg74 Jul 5, 2022
47ff267
Update the imported LLVMGENXIntrinsics
fineg74 Jun 17, 2022
560a38a
Merge remote-tracking branch 'origin/sycl' into DynamicLibBuild
fineg74 Jul 7, 2022
6296046
Address PR comments
fineg74 Jul 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 34 additions & 13 deletions llvm-spirv/lib/SPIRV/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
add_llvm_library(LLVMSPIRVLib
set(SRC_LIST
LLVMSPIRVOpts.cpp
LLVMToSPIRVDbgTran.cpp
Mangler/FunctionDescriptor.cpp
Expand Down Expand Up @@ -35,20 +35,41 @@ add_llvm_library(LLVMSPIRVLib
libSPIRV/SPIRVStream.cpp
libSPIRV/SPIRVType.cpp
libSPIRV/SPIRVValue.cpp
LINK_COMPONENTS
Analysis
BitWriter
CodeGen
Core
Demangle
IRReader
Linker
Passes
Support
TransformUtils
)
if(LLVM_LINK_LLVM_DYLIB)
Copy link
Contributor

@bader bader Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

@fineg74 fineg74 Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@pvchupin pvchupin Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bader bader Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

add_llvm_library(LLVMSPIRVLib STATIC DISABLE_LLVM_LINK_LLVM_DYLIB
${SRC_LIST}
DEPENDS
intrinsics_gen
LLVMAnalysis
LLVMBitWriter
LLVMCodeGen
LLVMCore
LLVMDemangle
LLVMIRReader
LLVMLinker
LLVMPasses
LLVMSupport
LLVMTransformUtils
)
else()
add_llvm_library(LLVMSPIRVLib
${SRC_LIST}
LINK_COMPONENTS
Analysis
BitWriter
CodeGen
Core
Demangle
IRReader
Linker
Passes
Support
TransformUtils
DEPENDS
intrinsics_gen
)
)
endif()

target_include_directories(LLVMSPIRVLib
PRIVATE
Expand Down
8 changes: 4 additions & 4 deletions llvm/lib/SYCLLowerIR/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ endif()
if (NOT TARGET LLVMGenXIntrinsics)
if (NOT DEFINED LLVMGenXIntrinsics_SOURCE_DIR)
set(LLVMGenXIntrinsics_GIT_REPO https://github.com/intel/vc-intrinsics.git)
# Author: Haohai Wen <haohai.wen@intel.com>
# Date: Fri Jun 10 03:46:22 2022 +0000
# Format InstructionSimplify names
set(LLVMGenXIntrinsics_GIT_TAG abce9184b7a3a7fe1b02289b9285610d9dc45465)
# Author: Gregory Fine <gregory.fine@intel.com>
# Date: Wed, 6 July 2022 16:00:33 +0000
# Add support for LLVM_LINK_LLVM_DYLIB option
set(LLVMGenXIntrinsics_GIT_TAG c456ff05bd8e11458772acef20e59ef18aa7876d)

message(STATUS "vc-intrinsics repo is missing. Will try to download it from ${LLVMGenXIntrinsics_GIT_REPO}")
include(FetchContent)
Expand Down
31 changes: 24 additions & 7 deletions llvm/tools/spirv-to-ir-wrapper/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,31 @@
set(LLVM_LINK_COMPONENTS
BinaryFormat
Core
Support
SPIRVLib
)
BinaryFormat
Core
Support
SPIRVLib
)

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

Copy link
Contributor

@bader bader Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@fineg74 fineg74 Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bader bader Jul 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@fineg74 fineg74 Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@bader bader Jul 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@fineg74 fineg74 Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bader bader Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@fineg74 fineg74 Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kbobrovs kbobrovs Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bader bader Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

include_directories(
${LLVM_EXTERNAL_LLVM_SPIRV_SOURCE_DIR}/include
)

add_llvm_tool(spirv-to-ir-wrapper
spirv-to-ir-wrapper.cpp
if(LLVM_LINK_LLVM_DYLIB)
add_llvm_tool(spirv-to-ir-wrapper
spirv-to-ir-wrapper.cpp
DEPENDS
LLVMSPIRVLib
)
else()
add_llvm_tool(spirv-to-ir-wrapper
spirv-to-ir-wrapper.cpp
)
endif()

target_link_libraries(spirv-to-ir-wrapper
PRIVATE
LLVMSPIRVLib
)