[compiler-rt][CMake] Pass all flags to _Float16 try-compile #133952
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
arichardson
reviewed
Apr 1, 2025
The try-compile mechanism requires that `CMAKE_REQUIRED_FLAGS` is a
space-separated string instead of a list of flags. The original code
expanded BUILTIN_FLAGS into CMAKE_REQUIRED_FLAGS as a space-separated
string and then would overwrite CMAKE_REQUIRED_FLAGS with
TARGET_${arch}_CFLAGS prepended to the unexpanded
BUILTIN_CFLAGS_${arch}. This resulted in the first two arguments being
passed into the try-compile invocation, but dropping the other arguments
listed in BUILTIN_CFLAGS_${arch}.
This patch takes the contents of both TARGET_${arch}_CLFAGS and
BUILTIN_FLAGS_${arch}, combines them into a single list, and then
joins that list as a space-separated string in `CMAKE_REQUIRED_FLAGS`.
`TARGET_${arch}_CFLAGS` is expanded into `CMAKE_REQUIRED_FLAGS`, which
is implicitly added to the flags by `check_compile_definition`. We don't
need to re-expand the `TARGET_${arch}_CFLAGS` variable again.
etcwilde
force-pushed
the
ewilde/compiler-rt-all-the-flags
branch
from
ac4e25c to
4e29148
Compare
Even better, it looks like llvm-project/compiler-rt/cmake/Modules/CompilerRTUtils.cmake Lines 94 to 103 in 1edb6b0 I've dropped adding those arguments entirely and it's only adding |
arichardson
approved these changes
Apr 3, 2025
petrhosek
reviewed
Apr 3, 2025
Comment on lines
860
to
861
| set(CMAKE_REQUIRED_FLAGS ${BUILTIN_CFLAGS_${arch}} ${TARGET_${arch}_CFLAGS}) | ||
| list(JOIN CMAKE_REQUIRED_FLAGS " " CMAKE_REQUIRED_FLAGS) |
There was a problem hiding this comment.
This is going to overwrite CMAKE_REQUIRED_FLAGS set by parent directories. Instead, we should be concatenating the flags.
Suggested change
| set(CMAKE_REQUIRED_FLAGS ${BUILTIN_CFLAGS_${arch}} ${TARGET_${arch}_CFLAGS}) | |
| list(JOIN CMAKE_REQUIRED_FLAGS " " CMAKE_REQUIRED_FLAGS) | |
| set(BUILTIN_CFLAGS ${BUILTIN_CFLAGS_${arch}} ${TARGET_${arch}_CFLAGS}) | |
| list(JOIN BUILTIN_CFLAGS " " BUILTIN_CFLAGS) | |
| set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} ${BUILTIN_CFLAGS}") |
There was a problem hiding this comment.
The original behavior also did not include existing CMAKE_REQUIRED_FLAGS. Fixed.
The original behavior also omitted any existing required flags. Patch this to include them in the _Float16 support check.
petrhosek
approved these changes
Apr 4, 2025
arichardson
approved these changes
Apr 4, 2025
etcwilde
added a commit
to etcwilde/llvm-project
that referenced
this pull request
Apr 4, 2025
) The try-compile mechanism requires that `CMAKE_REQUIRED_FLAGS` is a space-separated string instead of a list of flags. The original code expanded `BUILTIN_FLAGS` into `CMAKE_REQUIRED_FLAGS` as a space-separated string and then would overwrite `CMAKE_REQUIRED_FLAGS` with `TARGET_${arch}_CFLAGS` prepended to the unexpanded `BUILTIN_CFLAGS_${arch}`. This resulted in the first two arguments being passed into the try-compile invocation, but dropping the other arguments listed in `BUILTIN_CFLAGS_${arch}`. This patch appends `TARGET_${arch}_CFLAGS` and `BUILTIN_CFLAGS_${arch}` to `CMAKE_REQUIRED_FLAGS` before expanding CMAKE_REQUIRED_FLAGS as a space-separated string. This passes any pre-set required flags, in addition to all of the builtin and target flags to the Float16 detection. (cherry picked from commit 0d3f5ec)
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/17/builds/7033 Here is the relevant piece of the build log for the reference |
devincoughlin
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Apr 21, 2025
) The try-compile mechanism requires that `CMAKE_REQUIRED_FLAGS` is a space-separated string instead of a list of flags. The original code expanded `BUILTIN_FLAGS` into `CMAKE_REQUIRED_FLAGS` as a space-separated string and then would overwrite `CMAKE_REQUIRED_FLAGS` with `TARGET_${arch}_CFLAGS` prepended to the unexpanded `BUILTIN_CFLAGS_${arch}`. This resulted in the first two arguments being passed into the try-compile invocation, but dropping the other arguments listed in `BUILTIN_CFLAGS_${arch}`. This patch appends `TARGET_${arch}_CFLAGS` and `BUILTIN_CFLAGS_${arch}` to `CMAKE_REQUIRED_FLAGS` before expanding CMAKE_REQUIRED_FLAGS as a space-separated string. This passes any pre-set required flags, in addition to all of the builtin and target flags to the Float16 detection. (cherry picked from commit 0d3f5ec) (cherry picked from commit e958952)
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.
The try-compile mechanism requires that
CMAKE_REQUIRED_FLAGSis a space-separated string instead of a list of flags. The original code expandedBUILTIN_FLAGSintoCMAKE_REQUIRED_FLAGSas a space-separated string and then would overwriteCMAKE_REQUIRED_FLAGSwithTARGET_${arch}_CFLAGSprepended to the unexpandedBUILTIN_CFLAGS_${arch}. This resulted in the first two arguments being passed into the try-compile invocation, but dropping the other arguments listed inBUILTIN_CFLAGS_${arch}.This patch appends
${TARGET_${arch}_CFLAGS}to the list, and then joins the elements with spaces, resulting in all arguments passed viaCMAKE_REQUIRED_FLAGSbeing space-separated and being used in the try-compile when detecting Float16.