[libspirv] Fix recent conflict resolutions #19401
Closed
+18
−93
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 combines some fixes for acos/asin/modf and some CMake fixes. The three maths functions were using an old macro that upstream renamed. Upstream also made the output directory of the final libclc files explicit with a variable, instead of relying on the current binary dir. This was in preparation for the ability for upstream to place libclc directly into clang's resource directory; see 81e6552. Downstream we'd already done something similar, only we were relying on placing libclc files in `${LLVM_LIBRARY_OUTPUT_INTDIR}/clc`. The tests were also relying on this. During the pulldown we were left with bits of CMake code using the upstream location and other bits using the downstream location. This commit unifies all under the upstream variable name, but temporarily adjusts it to point to the downstream location. Over time I am hoping that upstream and downstream will align on the same location for libclc library files. This commit also removes some now-unused CMake functions. After f14620a libdevice no longer uses libclc's CMake functions. This meant that we are now able to align closer to upstream. Our downstream `link_bc` had some divergence due to libdevice, and upstream doesn't use `process_bc`. Indeed, we weren't using it either after a recent pulldown. Lastly, upstream doesn't use `add_libclc_alias` and neither do we. It's possible that this will temporarily break on Windows due to us not including some old downstream divergence which changes symlinks to copies on Windows, but upstream 9b5959d has been landed so we can either wait to pull that in or cherry-pick it. Either way we are left with far fewer divergences from upstream so should make libclc more maintainable.
frasercrmck
requested review from
jsji,
wenju-he,
Maetveis,
bwyma and
mikolaj-pirog
|
I already pushed a patch I was working on to fix the test failures before I saw this. It will conflict. |
|
This AddLibclc.cmake does not build properly because the remangled versions depend upon ${builtins_lib} which is not defined within ${LIBCLC_OUTPUT_LIBRARY_DIR}. These should be updated to ${libclc_builtins_lib}. I will include this change in the patch. |
bwyma
added a commit
to bwyma/intel-llvm
that referenced
this pull request
Jul 11, 2025
This includes all of the changes from intel#19401 and resolves the conflict with 17fe1b5.
bwyma
added a commit
that referenced
this pull request
Jul 13, 2025
jchlanda
approved these changes
Jul 14, 2025
|
Superceded by #19410 |
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 combines some fixes for acos/asin/modf and some CMake fixes.
The three maths functions were using an old macro that upstream renamed.
Upstream also made the output directory of the final libclc files explicit with a variable, instead of relying on the current binary dir. This was in preparation for the ability for upstream to place libclc directly into clang's resource directory; see 81e6552.
Downstream we'd already done something similar, only we were relying on placing libclc files in
${LLVM_LIBRARY_OUTPUT_INTDIR}/clc. The tests were also relying on this. During the pulldown we were left with bits of CMake code using the upstream location and other bits using the downstream location. This commit unifies all under the upstream variable name, but temporarily adjusts it to point to the downstream location. Over time I am hoping that upstream and downstream will align on the same location for libclc library files.This commit also removes some now-unused CMake functions. After f14620a libdevice no longer uses libclc's CMake functions. This meant that we are now able to align closer to upstream. Our downstream
link_bchad some divergence due to libdevice, and upstream doesn't useprocess_bc. Indeed, we weren't using it either after a recent pulldown. Lastly, upstream doesn't useadd_libclc_aliasand neither do we. It's possible that this will temporarily break on Windows due to us not including some old downstream divergence which changes symlinks to copies on Windows, but upstream 9b5959d has been landed so we can either wait to pull that in or cherry-pick it. Either way we are left with far fewer divergences from upstream so should make libclc more maintainable.