-
Notifications
You must be signed in to change notification settings - Fork 753
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.
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
[SYCL] Link and include LLVM utility headers in SYCL library #16763
base: sycl
Are you sure you want to change the base?
Changes from all commits
a89edc6
4b479b2
cb78c5e
8eedefa
c8a5bf9
a1c5055
524e961
0a5c250
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,9 +30,7 @@ function(add_sycl_rt_library LIB_NAME LIB_OBJ_NAME) | |
if (SYCL_ENABLE_STACK_PRINTING) | ||
if(NOT MSVC OR (CMAKE_BUILD_TYPE STREQUAL "Debug" AND ARG_COMPILE_OPTIONS MATCHES ".*MDd.*") OR | ||
(NOT CMAKE_BUILD_TYPE STREQUAL "Debug" AND NOT ARG_COMPILE_OPTIONS MATCHES ".*MDd.*")) | ||
add_dependencies(${LIB_NAME} LLVMSupport) | ||
target_compile_definitions(${LIB_OBJ_NAME} PUBLIC ENABLE_STACK_TRACE) | ||
target_link_libraries(${LIB_NAME} PRIVATE LLVMSupport) | ||
endif() | ||
endif() | ||
|
||
|
@@ -147,6 +145,18 @@ function(add_sycl_rt_library LIB_NAME LIB_OBJ_NAME) | |
PRIVATE OpenCL-Headers | ||
) | ||
|
||
# Link with LLVMSupport and LLVMObject for shared utilities. | ||
add_dependencies(${LIB_NAME} LLVMSupport LLVMObject) | ||
target_include_directories(${LIB_NAME} SYSTEM PRIVATE ${LLVM_MAIN_INCLUDE_DIR}) | ||
target_link_libraries(${LIB_NAME} PRIVATE LLVMSupport LLVMObject) | ||
target_compile_definitions(${LIB_NAME} PRIVATE LLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING=1) | ||
|
||
if (NOT MSVC) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC, you can mark headers as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh neat! Much better. 😄 |
||
# LLVM library increases the size of the SYCL library quite significantly | ||
# without the following link options. | ||
target_link_options(${LIB_NAME} PRIVATE -Wl,--gc-sections) | ||
endif() | ||
|
||
if(SYCL_ENABLE_EXTENSION_JIT) | ||
if(NOT DEFINED LLVM_EXTERNAL_SYCL_JIT_SOURCE_DIR) | ||
message(FATAL_ERROR "Undefined LLVM_EXTERNAL_SYCL_JIT_SOURCE_DIR variable: Must be set when extension to JIT SYCL kernels is enabled") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm skeptical about linking LLVM libraries with SYCL RT. I had a similar discussion with @stdale-intel and @AlexeySachkov while implementing device image compression (like: #15124 (comment)) and one of the major concerns was potential version mismatch between LLVM libraries linked to SYCL RT and other components like
ocloc
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spoke to @AlexeySachkov about this and I agree that it could potentially be a problem if we were to link with it dynamically, but I doubt we would ever want to do that. We already ship enough libraries, and I don't imagine
ocloc
links dynamically with the LLVM libraries either. The only reason we want to link with these are to have some common utilities, so the binary size increase should also be minor as we link privately and won't leak any of the symbols from the libraries.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking LLVM libraries statically still can be a problem if we export any LLVM symbols from the SYCL runtime library. AFAIK, we have some measures to force SYCL runtime to export only explicitly marked symbols, but I would explicitly test compatibly with the projects linking against both SYCL runtime and LLVM libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do believe that should be avoided due to the
PRIVATE
linkage. We should also see it in our ABI symbols tests if there were any new symbols added to the binary after this link, but it seems to not be the case on neither Windows nor Linux.