[libclc] Switch to LLVM_RUNTIME_TARGETS build#21843
[libclc] Switch to LLVM_RUNTIME_TARGETS build#21843wenju-he wants to merge 4 commits intointel:sycl-webfrom
Conversation
Following 121f5a9, this PR removes deprecated `LLVM_ENABLE_RUNTIMES=libclc` build approach from SYCL toolchain. For nvptx64-nvidia-cuda build, pass following options to cmake configure: -DRUNTIMES_nvptx64-nvidia-cuda_LLVM_ENABLE_RUNTIMES=libclc -DLLVM_RUNTIME_TARGETS="nvptx64-nvidia-cuda"
wenju-he
requested review from
a team,
Maetveis and
cperkinsintel
as code owners
Maetveis
left a comment
Maetveis
left a comment
There was a problem hiding this comment.
Thanks! This is great because it simplifies a lot of things for libclc. I think those simplifications should be in scope for this PR, I've marked what I found:
| @@ -39,18 +35,18 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} ) | |||
| # The solution is to create a separate directory for each target, | |||
| # and copy the lit.site.cfg.py file there; the name of the | |||
| # directory will be used as the target name. | |||
| file(MAKE_DIRECTORY ${LIBCLC_PERTARGET_TEST_DIR}/${t}) | |||
| file(MAKE_DIRECTORY ${LIBCLC_PERTARGET_TEST_DIR}/${LIBCLC_TARGET}) | |||
| file(COPY_FILE ${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg.py | |||
| ${LIBCLC_PERTARGET_TEST_DIR}/${t}/lit.site.cfg.py) | |||
| ${LIBCLC_PERTARGET_TEST_DIR}/${LIBCLC_TARGET}/lit.site.cfg.py) | |||
|
|
|||
| add_lit_testsuite(check-libclc-spirv-${t} | |||
| "Running libclc spirv-${t} regression tests" | |||
| ${LIBCLC_PERTARGET_TEST_DIR}/${t} | |||
| add_lit_testsuite(check-libclc-spirv-${LIBCLC_TARGET} | |||
| "Running libclc spirv-${LIBCLC_TARGET} regression tests" | |||
| ${LIBCLC_PERTARGET_TEST_DIR}/${LIBCLC_TARGET} | |||
| DEPENDS | |||
| ${LIBCLC_TEST_DEPS} | |||
| libspirv-builtins | |||
| ) | |||
| endforeach( t ) | |||
| endif() | |||
There was a problem hiding this comment.
If we no longer support multiple targets in the same build then this whole block should go away completely:
if(ARCH MATCHES "amdgcn|nvptx64")
add_lit_testsuite(check-libclc-spirv-${LIBCLC_TARGET}
"Running libclc spirv-${LIBCLC_TARGET} regression tests"
${CMAKE_CURRENT_BINARY_DIR}
DEPENDS
${LIBCLC_TEST_DEPS}
libspirv-builtins
)
endif()There was a problem hiding this comment.
we no longer support multiple targets in the same build
This is correct.
this block is testing libspirv for when ARCH is either amdgcn or nvptx64 target. why delete it?
There was a problem hiding this comment.
LIBCLC_TARGET in above block will be replaced with library target when testing remangled libspirv as well.
There was a problem hiding this comment.
LIBCLC_PERTARGET_TEST_DIR should be removed.
We don't need per-target test dirs if we only have one target in each build of libclc now.
There was a problem hiding this comment.
LIBCLC_PERTARGET_TEST_DIRshould be removed.
Removed.
We don't need per-target test dirs if we only have one target in each build of libclc now.
We probably still need different dirs when one target has multiple libraries.
| config.target_triple = None | ||
|
|
There was a problem hiding this comment.
Why are we setting this exactly? Where is it used?
There was a problem hiding this comment.
config.target_triple is required for below use_clang call. I have changed config.target_triple = None to config.target_triple = config.libclc_target
| if config.libclc_target is None: | ||
| lit_config.fatal("libclc_target parameter must be set when running directly") |
There was a problem hiding this comment.
This can also be removed libclc_target should never be None anymore.
| # test_exec_root: The root path where tests should be run. We create a unique | ||
| # test directory per libclc target to test to avoid data races when multiple | ||
| # targets try and access the the same libclc test files. | ||
| config.test_exec_root = os.path.join( | ||
| config.libclc_pertarget_test_dir, config.libclc_target | ||
| ) |
There was a problem hiding this comment.
This also needs to be modified, we only have one target now in each build so we shouldn't need to do this.
There was a problem hiding this comment.
will keep it. There could be multiple libraries per one targets. For instance, libclc_target can be different when we testing remangled libspirv as well in the future.
There was a problem hiding this comment.
This block should be removed too.
cperkinsintel
left a comment
cperkinsintel
left a comment
There was a problem hiding this comment.
sycl and sycl-jit LGTM
3 participants
Following 121f5a9, this PR removes deprecated
LLVM_ENABLE_RUNTIMES=libclcbuild approach from SYCL toolchain.For nvptx64-nvidia-cuda build, pass following options to cmake configure:
-DRUNTIMES_nvptx64-nvidia-cuda_LLVM_ENABLE_RUNTIMES=libclc
-DLLVM_RUNTIME_TARGETS="nvptx64-nvidia-cuda"
Submit this PR to sycl-web to avoid a gap with a downstream branch
which has transitioned to LLVM_RUNTIME_TARGETS build.