Skip to content

[SYCL RTC] Use cmake --install for resource.cpp generation #20466

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

Merged
merged 7 commits into from
Oct 28, 2025
Merged

[SYCL RTC] Use cmake --install for resource.cpp generation #20466

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c7597b7
[SYCL RTC] Use `cmake --install` for `resource.cpp` generation
aelovikov-intel Oct 24, 2025
73e9f6e
SYCL_JIT_RESOURCE_INSTALL_DIR
aelovikov-intel Oct 24, 2025
2b47019
s/ALL_RTC_PREPARE_RESOURCES_COMMANDS/SYCL_JIT_PREPARE_RESOURCE_COMMANDS/
aelovikov-intel Oct 24, 2025
ce42833
Drop COMMAND_EXPAND_LISTS
aelovikov-intel Oct 24, 2025
648c87a
Drop now useless comments
aelovikov-intel Oct 24, 2025
7dc63ba
Re-use components for deps
aelovikov-intel Oct 27, 2025
7598d3e
Better track incremental build deps
aelovikov-intel Oct 27, 2025
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
26 changes: 20 additions & 6 deletions sycl-jit/jit-compiler/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,39 @@ else()
set(SYCL_JIT_VIRTUAL_TOOLCHAIN_ROOT "/sycl-jit-toolchain/")
endif()

set(SYCL_JIT_RESOURCE_DEPS
sycl-headers # include/sycl
clang # lib/clang/N/include
${CMAKE_CURRENT_SOURCE_DIR}/utils/generate.py)
set(SYCL_JIT_RESOURCE_DEPS sycl-headers clang)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we don't need to depends on OpenCL-Headers?
Also do we need to depends on clang just for clang-resource-headers?

If we can depend on all the components to be installed, then we can just maintain one list instead two lists here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why we don't need to depends on OpenCL-Headers?

There is no such target, but I probably need to dig deeper still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clang-resource-headers is enough. Not sure what's actual underlying build target for OpenCL-Headers, but I think it's very safe to assume it's built by one of other dependencies (i.e., ninja sycl-jit passes in a clean build). @jsji , would you take another look?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Yeah, looks like we did not create target for OpenCL-Headers

if(NOT OpenCL_FOUND)
  # Copy OpenCL Headers into sycl headers build directory
  # Compiler does automatic lookup bin/../include based on clang binary location,
  # e.g. when run LIT tests
  file(COPY ${OpenCL_INCLUDE_DIR}/CL DESTINATION ${SYCL_INCLUDE_BUILD_DIR})

  # Include OpenCL Headers into final bundle.
  install(DIRECTORY ${OpenCL_INCLUDE_DIR}/CL
          DESTINATION ${SYCL_INCLUDE_DIR}
          COMPONENT OpenCL-Headers)
endif()

So we either need to create a target for it, or we can assume OpenCL-Headers is always there, so no dependency is needed.

set(SYCL_JIT_RESOURCE_INSTALL_COMPONENTS sycl-headers OpenCL-Headers clang-resource-headers)

if ("libclc" IN_LIST LLVM_ENABLE_PROJECTS)
# Somehow just "libclc" doesn't build "remangled-*" (and maybe whatever else).
list(APPEND SYCL_JIT_RESOURCE_DEPS libclc libspirv-builtins) # lib/clc/*.bc
list(APPEND SYCL_JIT_RESOURCE_INSTALL_COMPONENTS libspirv-builtins)
endif()

if ("libdevice" IN_LIST LLVM_ENABLE_PROJECTS)
list(APPEND SYCL_JIT_RESOURCE_DEPS libsycldevice) # lib/*.bc
list(APPEND SYCL_JIT_RESOURCE_INSTALL_COMPONENTS libsycldevice)
endif()

set(ALL_RTC_PREPARE_RESOURCES_COMMANDS)
foreach(component IN LISTS SYCL_JIT_RESOURCE_INSTALL_COMPONENTS)
list(APPEND ALL_RTC_PREPARE_RESOURCES_COMMANDS
COMMAND ${CMAKE_COMMAND} --install ${CMAKE_BINARY_DIR} --prefix ${CMAKE_CURRENT_BINARY_DIR}/rtc-resources-install --component "${component}"
Copy link
Contributor

Choose a reason for hiding this comment

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

So the idea here is we re-install all these components into a new rtc-resources-install subfolder and then run that script to embed them into something, and we need to do the separate install because using the actual installed files runs into race conditions because files are moved around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, except that before this PR we weren't even installing - just taking from the build directly.

Also, race is only one issue, better control of what's in there is beneficial on its own.

)
endforeach()

add_custom_target(rtc-prepare-resources
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like add_custom_target is always considered out of date, so this will be run any time someone builds even if it's incremental. Is that a big deal? Is this expensive to run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that a big deal?

Not sure

Is this expensive to run?

No (at least IMO), and the incremental builds will output this:

-- Up-to-date: <root>/build/tools/sycl-jit/jit-compiler/rtc-resources-install/lib/libsycl-fallback-imf-fp64.o

instead of

-- Installing: <root>/build/tools/sycl-jit/jit-compiler/rtc-resources-install/lib/libsycl-fallback-imf-fp64.o

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also thinking about rm -rf rtc-resources-install after resource.cpp is generated so that we won't have stale files in there. In that case we'd even want to run it all the time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, it will just call the target which itself will determine it is up to date, so basically nothing happens and it's better than before IMO

Copy link
Contributor

@sarnex sarnex Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm also thinking about rm -rf rtc-resources-install after resource.cpp is generated so that we won't have stale files in there. In that case we'd even want to run it all the time.

Wouldn't the version of the file in that folder be regenerated if anyone makes a change to the file? Like someone modifies sycl.h and rebuilds and at that point it will rerun the resource.cpp generation in full because dependency sycl-headers or whatever changed right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating sycl.hpp works with this version (AFAIK), I was thinking about deleting it. But, to be fair, we have that problem everywhere else, so probably no reason to address here.

DEPENDS ${SYCL_JIT_RESOURCE_DEPS}
${ALL_RTC_PREPARE_RESOURCES_COMMANDS}
COMMAND_EXPAND_LISTS
)

add_custom_command(
OUTPUT ${SYCL_JIT_RESOURCE_CPP}
COMMAND ${Python3_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/utils/generate.py --toolchain-dir ${CMAKE_BINARY_DIR} --output ${SYCL_JIT_RESOURCE_CPP} --prefix ${SYCL_JIT_VIRTUAL_TOOLCHAIN_ROOT}
COMMAND ${Python3_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/utils/generate.py --toolchain-dir ${CMAKE_CURRENT_BINARY_DIR}/rtc-resources-install --output ${SYCL_JIT_RESOURCE_CPP} --prefix ${SYCL_JIT_VIRTUAL_TOOLCHAIN_ROOT}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could we make rtc-resources-install into a variable so its not copy pasted here and in the loop defining it?

DEPENDS
${SYCL_JIT_RESOURCE_DEPS}
rtc-prepare-resources
${CMAKE_CURRENT_SOURCE_DIR}/utils/generate.py
)

# We use C23/C++26's `#embed` to implement this resource creation, and "current"
Expand Down
10 changes: 1 addition & 9 deletions sycl-jit/jit-compiler/utils/generate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,7 @@ def process_dir(dir):
file_path = os.path.join(root, file)
process_file(file_path)

process_dir(os.path.join(args.toolchain_dir, "include/"))
process_dir(os.path.join(args.toolchain_dir, "lib/clang/"))
process_dir(os.path.join(args.toolchain_dir, "lib/clc/"))

for file in glob.iglob(
"*.bc", root_dir=os.path.join(args.toolchain_dir, "lib")
):
file_path = os.path.join(args.toolchain_dir, "lib", file)
process_file(file_path)
process_dir(args.toolchain_dir)

out.write(
f"""
Expand Down
Loading