Skip to content

[builtins] Fix missing main() function in float16/bfloat16 support checks #104478

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

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

overmighty
Copy link
Member

@overmighty overmighty commented Aug 15, 2024

The CMake docs state that check_c_source_compiles() checks whether the
supplied code "can be compiled as a C source file and linked as an
executable (so it must contain at least a main() function)."

https://cmake.org/cmake/help/v3.30/module/CheckCSourceCompiles.html

In practice, this command is a wrapper around try_compile():

When CMAKE_SOURCE_DIR is compiler-rt/lib/builtins/,
CMAKE_TRY_COMPILE_TARGET_TYPE is set to STATIC_LIBRARY, so the
checks for float16 and bfloat16 support work as intended in a
Clang + compiler-rt runtime build for example, as it runs CMake
recursively from that directory.

However, when using llvm/ or compiler-rt/ as CMake source directory, as
CMAKE_TRY_COMPILE_TARGET_TYPE defaults to EXECUTABLE, these checks
will indeed fail if the code doesn't have a main() function. This
results in LLVM using x86 SIMD registers when generating calls to
builtins that, with Arch Linux's compiler-rt package for example,
actually use a GPR for their argument or return value as they use
uint16_t instead of _Float16.

This had been caught in post-commit review:
https://reviews.llvm.org/D145237#4521152. Use of the internal
CMAKE_C_COMPILER_WORKS variable is not what hides the issue, however.

PR #69842 tried to fix this by unconditionally setting
CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY, but it apparently
caused other issues, so it was reverted. This PR just adds a main()
function in the checks, as per the CMake docs.

…ecks

The CMake docs state that `check_c_source_compiles()` checks whether the
supplied code "can be compiled as a C source file and linked as an
executable (so it must contain at least a main() function)."

https://cmake.org/cmake/help/v3.30/module/CheckCSourceCompiles.html

In practice, this command is a wrapper around `try_compile()`:

- https://github.com/Kitware/CMake/blob/2904ce00d2ed6ad5dac6d3459af62d8223e06ce0/Modules/CheckCSourceCompiles.cmake#L54
- https://github.com/Kitware/CMake/blob/2904ce00d2ed6ad5dac6d3459af62d8223e06ce0/Modules/Internal/CheckSourceCompiles.cmake#L101

When `CMAKE_SOURCE_DIR` is compiler-rt/lib/builtins/,
`CMAKE_TRY_COMPILE_TARGET_TYPE` is set to `STATIC_LIBRARY`, so the
checks for `float16` and `bfloat16` support work as expected in a
Clang + compiler-rt runtime build for example, as it runs CMake
recursively from that directory.

However, when using llvm/ or compiler-rt/ as CMake source directory, as
`CMAKE_TRY_COMPILE_TARGET_TYPE` defaults to `EXECUTABLE`, these checks
will indeed fail if the code doesn't have a `main()` function. On Arch
Linux, this results in Clang using x86 SIMD registers when calling
builtins that, with compiler-rt, actually use GPRs as they use
`uint16_t` instead of `_Float16`.

This had been caught in post-commit review:
https://reviews.llvm.org/D145237#4521152. Use of the internal
`CMAKE_C_COMPILER_WORKS` variable is not what hides the issue, however.

PR llvm#69842 tried to fix this by unconditionally setting
`CMAKE_TRY_COMPILE_TARGET_TYPE` to `STATIC_LIBRARY`, but it apparently
caused other issues, so it was reverted. This PR just adds a `main()`
function in the checks, as per the CMake docs.
@overmighty overmighty requested a review from petrhosek August 19, 2024 17:13
@overmighty overmighty merged commit 68d8b38 into llvm:main Aug 30, 2024
10 checks passed
@kraj
Copy link
Contributor

kraj commented Aug 30, 2024

as mentioned in #83639 please backport it into 18.x as well.

@overmighty
Copy link
Member Author

There won't be another LLVM 18.x release, but I guess we could backport it into 19.x at least. See #101358 (comment).

@overmighty overmighty added this to the LLVM 19.X Release milestone Aug 31, 2024
@overmighty
Copy link
Member Author

/cherry-pick 68d8b38

@overmighty overmighty added the cmake Build system in general and CMake in particular label Aug 31, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 31, 2024
…ecks (llvm#104478)

The CMake docs state that `check_c_source_compiles()` checks whether the
supplied code "can be compiled as a C source file and linked as an
executable (so it must contain at least a `main()` function)."

https://cmake.org/cmake/help/v3.30/module/CheckCSourceCompiles.html

In practice, this command is a wrapper around `try_compile()`:

- https://gitlab.kitware.com/cmake/cmake/blob/2904ce00d2ed6ad5dac6d3459af62d8223e06ce0/Modules/CheckCSourceCompiles.cmake#L54
- https://gitlab.kitware.com/cmake/cmake/blob/2904ce00d2ed6ad5dac6d3459af62d8223e06ce0/Modules/Internal/CheckSourceCompiles.cmake#L101

When `CMAKE_SOURCE_DIR` is compiler-rt/lib/builtins/,
`CMAKE_TRY_COMPILE_TARGET_TYPE` is set to `STATIC_LIBRARY`, so the
checks for `float16` and `bfloat16` support work as intended in a
Clang + compiler-rt runtime build for example, as it runs CMake
recursively from that directory.

However, when using llvm/ or compiler-rt/ as CMake source directory, as
`CMAKE_TRY_COMPILE_TARGET_TYPE` defaults to `EXECUTABLE`, these checks
will indeed fail if the code doesn't have a `main()` function. This
results in LLVM using x86 SIMD registers when generating calls to
builtins that, with Arch Linux's compiler-rt package for example,
actually use a GPR for their argument or return value as they use
`uint16_t` instead of `_Float16`.

This had been caught in post-commit review:
https://reviews.llvm.org/D145237#4521152. Use of the internal
`CMAKE_C_COMPILER_WORKS` variable is not what hides the issue, however.

PR llvm#69842 tried to fix this by unconditionally setting
`CMAKE_TRY_COMPILE_TARGET_TYPE` to `STATIC_LIBRARY`, but it apparently
caused other issues, so it was reverted. This PR just adds a `main()`
function in the checks, as per the CMake docs.

(cherry picked from commit 68d8b38)
@llvmbot
Copy link
Member

llvmbot commented Aug 31, 2024

/pull-request #106843

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Sep 1, 2024
…ecks (llvm#104478)

The CMake docs state that `check_c_source_compiles()` checks whether the
supplied code "can be compiled as a C source file and linked as an
executable (so it must contain at least a `main()` function)."

https://cmake.org/cmake/help/v3.30/module/CheckCSourceCompiles.html

In practice, this command is a wrapper around `try_compile()`:

- https://gitlab.kitware.com/cmake/cmake/blob/2904ce00d2ed6ad5dac6d3459af62d8223e06ce0/Modules/CheckCSourceCompiles.cmake#L54
- https://gitlab.kitware.com/cmake/cmake/blob/2904ce00d2ed6ad5dac6d3459af62d8223e06ce0/Modules/Internal/CheckSourceCompiles.cmake#L101

When `CMAKE_SOURCE_DIR` is compiler-rt/lib/builtins/,
`CMAKE_TRY_COMPILE_TARGET_TYPE` is set to `STATIC_LIBRARY`, so the
checks for `float16` and `bfloat16` support work as intended in a
Clang + compiler-rt runtime build for example, as it runs CMake
recursively from that directory.

However, when using llvm/ or compiler-rt/ as CMake source directory, as
`CMAKE_TRY_COMPILE_TARGET_TYPE` defaults to `EXECUTABLE`, these checks
will indeed fail if the code doesn't have a `main()` function. This
results in LLVM using x86 SIMD registers when generating calls to
builtins that, with Arch Linux's compiler-rt package for example,
actually use a GPR for their argument or return value as they use
`uint16_t` instead of `_Float16`.

This had been caught in post-commit review:
https://reviews.llvm.org/D145237#4521152. Use of the internal
`CMAKE_C_COMPILER_WORKS` variable is not what hides the issue, however.

PR llvm#69842 tried to fix this by unconditionally setting
`CMAKE_TRY_COMPILE_TARGET_TYPE` to `STATIC_LIBRARY`, but it apparently
caused other issues, so it was reverted. This PR just adds a `main()`
function in the checks, as per the CMake docs.

(cherry picked from commit 68d8b38)
@brooksdavis
Copy link
Contributor

Unfortunately the fix seems to have traded a false negative for a false positive when building compiler-rt on FreeBSD amd64. The 32-bit build then fails with:

<...>/llvm-project-19.1.0-rc4.src/compiler-rt/lib/builtins/fp_extend.h:57:9: error: _Float16 is not supported on this target
   57 | typedef _Float16 src_t;
      |         ^
1 error generated.

@mstorsjo
Copy link
Member

@petrhosek This issue above, and the issue described at f12cdda#r1559768981, both seem to be caused by compiler-rt building internally for multiple architectures, while the cmake tests only run once. If the tested properties differ between the multiple architectures, like when compiler-rt implicitly builds for both i386 and x86_64 at the same time, this fails, if we e.g. have libunwind in the x86_64 environment but not in the i386 one, or like here, if the x86_64 environment supports _Float16 but the i386 one doesn't.

The only way around this, as far as I see it, is to move the implicit compiler-rt multiarch support out and do a proper runtime build for each of the architectures, with a separate (nested) cmake invocation and checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular compiler-rt:builtins compiler-rt release:backport
Projects
Development

Successfully merging this pull request may close these issues.

6 participants