[SYCL][libclc] Fix missing build dependencies #13145
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit aims to fix the problem of missing build dependencies between libclc source files and their various includes. We would like to do this with compiler-generated dependency files because then the dependencies are accurate and there are no false positives, leading to unnecessary rebuilds. This is how regular C/C++ dependencies are usually tracked by CMake. Note that this variable is an internal API so is not guaranteed to work, but then again *all* of CMake's support for new languages (which we use for CLC/LL languages) is an internal API. On balance this change is probably worth it due to how minimally invasive it is. The alternative would be to either: 1. list/glob all possible files in the directory as dependencies, which would lead to false positives. 2. rewrite the library generation as a loop over all files and calling `add_custom_command`, which can produce a dependency file (by tweaking our clang command line) that can also be fed back to the same command via the `DEPFILE` argument. This would be a much larger change and is not as "neat".
frasercrmck
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
ldrumm
approved these changes
Mar 25, 2024
There was a problem hiding this comment.
Since we're already setting CMAKE_INCLUDE_FLAG_CLC, I think we have precedent for using these internal interfaces.
In fact, since cmake has documentation on how to use these interfaces, I'd say this is better than the custom command option and should be at least as stable as maintaining all the extra code
LGTM
frasercrmck
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@intel/llvm-gatekeepers I think this can be merged - thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes the problem of missing build dependencies between libclc source files and their various includes.
We would like to do this with compiler-generated dependency files because then the dependencies are accurate and there are no false positives, leading to unnecessary rebuilds. This is how regular C/C++ dependencies are usually tracked by CMake.
Note that this variable is an internal API so is not guaranteed to work, but then again all of CMake's support for new languages (which we use for CLC/LL languages) is an internal API. On balance this change is probably worth it due to how minimally invasive it is.
The alternative would be to either:
add_custom_command, which can produce a dependency file (by tweaking our clang command line) that can also be fed back to the same command via theDEPFILEargument. This would be a much larger change and is not as "neat".