Skip to content
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.

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

[libclc] Track dependencies through dependency files #86965

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

frasercrmck
Copy link
Contributor

This commit fixes the problem of missing build dependencies between libclc source files and their various includes (namely headers and .inc files).

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. It should work with all supported compilers and CMake generators.

This commit fixes the problem of missing build dependencies between
libclc source files and their various includes (namely headers and .inc
files).

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. It should work
with all supported compilers and CMake generators.
@arsenm
Copy link
Contributor

arsenm commented Mar 28, 2024

The build here seems to be trying to define clc as a language, which then results in needing to rely on language support magic like this. I think it would be better if this did what rocm-device-libs does and treat these as custom targets. I don't think it's particularly helpful to pretend like this is a normal host compiler detection situation.

https://github.com/ROCm/llvm-project/blob/efc7219da809b6bce7b1c92c1915a0eac68fb1f2/amd/device-libs/cmake/OCL.cmake#L136

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

In the context of what the build is already doing, this should be fine

@frasercrmck
Copy link
Contributor Author

The build here seems to be trying to define clc as a language, which then results in needing to rely on language support magic like this. I think it would be better if this did what rocm-device-libs does and treat these as custom targets. I don't think it's particularly helpful to pretend like this is a normal host compiler detection situation.

https://github.com/ROCm/llvm-project/blob/efc7219da809b6bce7b1c92c1915a0eac68fb1f2/amd/device-libs/cmake/OCL.cmake#L136

Yes, I agree. It also makes building libclc in-tree a nuisance, because CMake needs to know the path to the "language compilers" at configure time, which leads to fun like intuiting the final output path of clang and other tools and writing an empty string to those paths to ensure they exist on disk before you call enable_language.

So I agree that moving to custom targets would be beneficial for many reasons. I think overall it would be a simpler solution, not least because it's the more idiomatic way of doing CMake. I might look into that in the near future, but can't promise it.

@frasercrmck frasercrmck merged commit 688d71e into llvm:main Mar 28, 2024
5 checks passed
@frasercrmck frasercrmck deleted the libclc-depfiles branch March 28, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants