[SYCL] Link and include LLVM utility headers in SYCL library #16763

steffenlarsen · 2025-01-24T11:43:02Z

These changes propose that the SYCL runtime library privately links against LLVM to avoid the need for replicating common utility code. In the initial version we can use this to remove the property-set names, making it more safely consistent.

These changes intend to aid the implementation of loading SYCL-based kernel binary files at runtime.

These changes propose that the SYCL runtime library privately links against LLVM to avoid the need for replicating common utility code. In the initial version we can use this to remove the property-set names, making it more safely consistent. These changes intend to aid the implementation of loading SYCL-based kernel binary files at runtime. Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen · 2025-01-24T11:44:55Z

@jopperm @bader @sommerlukas @cperkinsintel @AlexeySachkov @uditagarwal97 @sergey-semenov @aelovikov-intel - I realize that this change can be somewhat controversial, so please raise any concerns and we can discuss them.

Signed-off-by: Larsen, Steffen <[email protected]>

uditagarwal97 · 2025-01-24T16:58:35Z

sycl/source/CMakeLists.txt

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


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.

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.

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.

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.

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.

bader · 2025-01-24T17:59:55Z

sycl/source/CMakeLists.txt

@@ -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_OBJ_NAME} LLVMSupport LLVMObject)
+  target_include_directories(${LIB_OBJ_NAME} PRIVATE ${LLVM_MAIN_INCLUDE_DIR})


Is using PRIVATE keyword here enough to protect us from accidentally including LLVM headers into SYCL headers?

Based on local testing, 384 check-sycl tests start failing with "fatal error: 'llvm/X.h' file not found" if I introduce an include of an LLVM header in the non-source headers.

cperkinsintel · 2025-01-24T19:34:32Z

Besides PropertySetRegistry are there other LLVM utils you think we might/should leverage in the future?

steffenlarsen · 2025-01-27T05:57:52Z

Besides PropertySetRegistry are there other LLVM utils you think we might/should leverage in the future?

A future extension is going to introduce a file-format that both the runtime and the compiler tooling will need to be able to read/write. Being able to share the logic will simplify the code immensely.

sommerlukas

The general idea of sharing headers between compiler and runtime to avoid duplication that requires manual maintenance is good, I think.

Did you measure increase in library size? When we initially added sycl-jit, the library binary of libsycl.so was a concern, which is why we eventually switched to loading it dynamically.

sommerlukas · 2025-01-24T13:34:37Z

sycl/source/CMakeLists.txt

+  target_link_libraries(${LIB_OBJ_NAME} PRIVATE LLVMSupport LLVMObject)
+  target_compile_definitions(${LIB_OBJ_NAME} PRIVATE LLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING=1)
+
+  if (NOT MSVC)


IIRC, you can mark headers as SYSTEM in CMake to avoid warning being emitted for them. We do this with the LLVM headers in SYCL-JIT: https://github.com/intel/llvm/blob/sycl/sycl-jit/jit-compiler/CMakeLists.txt#L57-L63

Oh neat! Much better. 😄

steffenlarsen · 2025-01-27T13:43:37Z

Did you measure increase in library size? When we initially added sycl-jit, the library binary of libsycl.so was a concern, which is why we eventually switched to loading it dynamically.

I did initially try to, but must have failed. It does indeed seem like the library size increases quite a bit (~4x). However, if we add

    target_link_options(${LIB_NAME} PRIVATE -Wl,--gc-sections)

that seems to drop down to only an increase in size of about 8%. I can play with a few more options to see if there's more of it we can cut. I also need to check what the effect is on Windows and if there's a corresponding option there.

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen requested review from jopperm, bader, sommerlukas, cperkinsintel, AlexeySachkov, uditagarwal97, sergey-semenov and aelovikov-intel January 24, 2025 11:43

steffenlarsen had a problem deploying to WindowsCILock January 24, 2025 11:43 — with GitHub Actions Error

Disable unused parameter warnings in library

4b479b2

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen had a problem deploying to WindowsCILock January 24, 2025 11:58 — with GitHub Actions Failure

Fix lowercase private

cb78c5e

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen had a problem deploying to WindowsCILock January 24, 2025 12:06 — with GitHub Actions Error

Change to LIB_OBJ_NAME

8eedefa

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen had a problem deploying to WindowsCILock January 24, 2025 12:26 — with GitHub Actions Failure

Exclude warning flag on MSVC

c8a5bf9

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen temporarily deployed to WindowsCILock January 24, 2025 13:04 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to WindowsCILock January 24, 2025 13:35 — with GitHub Actions Inactive

uditagarwal97 reviewed Jan 24, 2025

View reviewed changes

bader reviewed Jan 24, 2025

View reviewed changes

sommerlukas reviewed Jan 27, 2025

View reviewed changes

steffenlarsen added 3 commits January 27, 2025 05:45

Address binary size on non-MSVC

a1c5055

Signed-off-by: Larsen, Steffen <[email protected]>

Fix to use LIB_NAME instead of LIB_OBJ_NAME

524e961

Signed-off-by: Larsen, Steffen <[email protected]>

Avoid need for disabling warnings from LLVM headers

0a5c250

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen temporarily deployed to WindowsCILock January 27, 2025 13:55 — with GitHub Actions Inactive

steffenlarsen had a problem deploying to WindowsCILock January 27, 2025 14:22 — with GitHub Actions Failure

steffenlarsen had a problem deploying to WindowsCILock January 27, 2025 15:52 — with GitHub Actions Failure

steffenlarsen had a problem deploying to WindowsCILock January 27, 2025 17:25 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Link and include LLVM utility headers in SYCL library #16763

[SYCL] Link and include LLVM utility headers in SYCL library #16763

steffenlarsen commented Jan 24, 2025

steffenlarsen commented Jan 24, 2025

uditagarwal97 Jan 24, 2025

steffenlarsen Jan 24, 2025

bader Jan 24, 2025

steffenlarsen Jan 27, 2025

bader Jan 24, 2025

steffenlarsen Jan 27, 2025

cperkinsintel commented Jan 24, 2025

steffenlarsen commented Jan 27, 2025

sommerlukas left a comment

sommerlukas Jan 24, 2025

steffenlarsen Jan 27, 2025

steffenlarsen commented Jan 27, 2025

[SYCL] Link and include LLVM utility headers in SYCL library #16763

Are you sure you want to change the base?

[SYCL] Link and include LLVM utility headers in SYCL library #16763

Conversation

steffenlarsen commented Jan 24, 2025

steffenlarsen commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cperkinsintel commented Jan 24, 2025

steffenlarsen commented Jan 27, 2025

sommerlukas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steffenlarsen commented Jan 27, 2025