[libclc] Only create a target per each compile command for cmake MSVC generator #154479
Conversation
… generator libclc sequential build issue addressed in commit 0c21d6b is specific to cmake MSVC generator. Therefore, this PR avoids creating a large number of targets when a non-MSVC generator is used, such as the Ninja generator, which is used in pre-merge CI on Windows in llvm-project and our downstream repos.
frasercrmck
left a comment
frasercrmck
left a comment
There was a problem hiding this comment.
Thanks for addressing this
| # * FILES <string> ... | ||
| # List of bitcode files | ||
| function(create_compile_targets compile_tgts) | ||
| cmake_parse_arguments( ARG "" "ARCH_SUFFIX" "FILES" ${ARGN} ) |
There was a problem hiding this comment.
Perhaps we should error if ARCH_SUFFIX is empty? Just thinking that compile--* looks unintentional. Maybe an empty FILES is okay, maybe not.
There was a problem hiding this comment.
there is already such check at
llvm-project/libclc/cmake/modules/AddLibclc.cmake
Lines 272 to 274 in 8cb6bfe
There was a problem hiding this comment.
I think it's best if functions are self-contained, but it's not a blocker.
There was a problem hiding this comment.
I think it's best if functions are self-contained, but it's not a blocker.
done, checked both ARCH_SUFFIX and FILES in a8d7ec9
| # * FILES <string> ... | ||
| # List of bitcode files | ||
| function(create_compile_targets compile_tgts) | ||
| cmake_parse_arguments( ARG "" "ARCH_SUFFIX" "FILES" ${ARGN} ) |
There was a problem hiding this comment.
I think it's best if functions are self-contained, but it's not a blocker.
3 participants
libclc sequential build issue addressed in commit 0c21d6b is
specific to cmake MSVC generator. Therefore, this PR avoids creating a
large number of targets when a non-MSVC generator is used, such as the
Ninja generator, which is used in pre-merge CI on Windows in
llvm-project repo. We plan to migrate from MSVC generator to Ninja
generator in our downstream CI to fix flaky cmake bug
Cannot restore timestamp, which might be related to the large number of targets.