Skip to content

[compiler-rt][builtins] Fix FLOAT16 feature detection #69842

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

littlewu2508
Copy link

I'm submitting this patch authored by @tstellar:

CMAKE_TRY_COMPILE_TARGET_TYPE defaults to EXECUTABLE, which causes any feature detection code snippet without a main function to fail, so we need to make sure it gets explicitly set to STATIC_LIBRARY.

I think maybe it should be up-streamed, since it causes some issues [1,2] in some distros. Please have a review to see whether this patch should be applied in upstream, or just in distro.

[1] ROCm/rocFFT#439
[2] ROCm/rocBLAS#1350

CMAKE_TRY_COMPILE_TARGET_TYPE defaults to EXECUTABLE, which causes
any feature detection code snippet without a main function to fail,
so we need to make sure it gets explicitly set to STATIC_LIBRARY.
@littlewu2508 littlewu2508 changed the title compiler-rt: Fix FLOAT16 feature detection [compiler-rt][builtins] Fix FLOAT16 feature detection Oct 21, 2023
@thesamesam thesamesam added the cmake Build system in general and CMake in particular label Oct 21, 2023
@thesamesam thesamesam requested review from mgorny and tstellar October 21, 2023 13:47
@tstellar
Copy link
Collaborator

This fixed a bug in the compiler-rt build configuration that we use in Fedora. I have no idea if this is correct for all build configurations.

@littlewu2508
Copy link
Author

This fixed a bug in the compiler-rt build configuration that we use in Fedora. I have no idea if this is correct for all build configurations.

I think, unless there's a reason that when CMAKE_SOURCE_DIR != CMAKE_CURRENT_SOURCE_DIR, which is the case for Fedora and Gentoo, it (in general) has to be non-static, then there's no reason we should put STATIC_LIBRARY inside that if branch.

And if there are scenarios STATIC_LIBRARY should not be set, then maybe the if branch excludes too much cases (Fedora and Gentoo's build configuration), unless our build configuration turned out to be somewhat "incompatible" or "wrong"

Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

LGTM.

@littlewu2508
Copy link
Author

Hi, @tstellar should we proceed merging this PR?

@tstellar
Copy link
Collaborator

@littlewu2508 What build configurations have you tested this with?

@littlewu2508
Copy link
Author

@littlewu2508 What build configurations have you tested this with?

Gentoo is building compiler-rt as a standalone project. The invoked cmake command on my machine is:

cmake -C /run/user/18014/portage/sys-libs/compiler-rt-17.0.4/work/compiler-rt-17.0.4_build/gentoo_common_config.cmake -G Ninja -DCMAKE_INSTALL_PREFIX=/opt/gentoo/usr -DCOMPILER_RT_INSTALL_PATH=/opt/gentoo/usr/lib/clang/17 -DCOMPILER_RT_INCLUDE_TESTS=no -DCOMPILER_RT_BUILD_LIBFUZZER=OFF -DCOMPILER_RT_BUILD_MEMPROF=OFF -DCOMPILER_RT_BUILD_ORC=OFF -DCOMPILER_RT_BUILD_PROFILE=OFF -DCOMPILER_RT_BUILD_SANITIZERS=OFF -DCOMPILER_RT_BUILD_XRAY=OFF -DPython3_EXECUTABLE=/opt/gentoo/usr/bin/python3.12 -DCAN_TARGET_i386=no -DCAN_TARGET_x86_64=yes -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_TOOLCHAIN_FILE=/run/user/18014/portage/sys-libs/compiler-rt-17.0.4/work/compiler-rt-17.0.4_build/gentoo_toolchain.cmake /run/user/18014/portage/sys-libs/compiler-rt-17.0.4/work/compiler-rt

And

gentoo_common_config.cmake is

set(CMAKE_GENTOO_BUILD ON CACHE BOOL "Indicate Gentoo package build")
set(LIB_SUFFIX 64 CACHE STRING "library path suffix" FORCE)
set(CMAKE_INSTALL_LIBDIR lib64 CACHE PATH "Output directory for libraries")
set(CMAKE_INSTALL_INFODIR "/opt/gentoo/usr/share/info" CACHE PATH "")
set(CMAKE_INSTALL_MANDIR "/opt/gentoo/usr/share/man" CACHE PATH "")
set(CMAKE_USER_MAKE_RULES_OVERRIDE "/run/user/18014/portage/sys-libs/compiler-rt-17.0.4/work/compiler-rt-17.0.4_build/gentoo_rules.cmake" CACHE FILEPATH "Gentoo override rules")
set(CMAKE_INSTALL_DOCDIR "/opt/gentoo/usr/share/doc/compiler-rt-17.0.4" CACHE PATH "")
set(BUILD_SHARED_LIBS ON CACHE BOOL "")
set(Python3_FIND_UNVERSIONED_NAMES FIRST CACHE STRING "")
set(CMAKE_INSTALL_ALWAYS 1)
set(CMAKE_ASM_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_ASM-ATT_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_C_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_Fortran_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_MODULE_LINKER_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_STATIC_LINKER_FLAGS_RELWITHDEBINFO "" CACHE STRING "")

while gentoo_toolchain.cmake is

set(CMAKE_ASM_COMPILER "x86_64-pc-linux-gnu-clang")
set(CMAKE_ASM-ATT_COMPILER "x86_64-pc-linux-gnu-clang")
set(CMAKE_C_COMPILER "x86_64-pc-linux-gnu-clang")
set(CMAKE_CXX_COMPILER "x86_64-pc-linux-gnu-clang++")
set(CMAKE_Fortran_COMPILER "x86_64-pc-linux-gnu-gfortran")
set(CMAKE_AR /opt/gentoo/usr/bin/x86_64-pc-linux-gnu-ar CACHE FILEPATH "Archive manager" FORCE)
set(CMAKE_RANLIB /opt/gentoo/usr/bin/x86_64-pc-linux-gnu-ranlib CACHE FILEPATH "Archive index generator" FORCE)
set(CMAKE_SYSTEM_PROCESSOR "x86_64")

heroxbd added a commit to heroxbd/gentoo that referenced this pull request Nov 19, 2023
heroxbd added a commit to heroxbd/gentoo that referenced this pull request Nov 19, 2023
@thesamesam
Copy link
Member

Now that 18 is branched, I think we should just try this.

@gulfemsavrun
Copy link
Contributor

gulfemsavrun commented Jan 24, 2024

We started seeing SanitizerCommon-tsan test failures after this patch in our Clang Linux builders.

FAIL: SanitizerCommon-tsan-x86_64-Linux :: Linux/abort_on_error.cpp (3645 of 15728)
******************** TEST 'SanitizerCommon-tsan-x86_64-Linux :: Linux/abort_on_error.cpp' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 4: /b/s/w/ir/x/w/llvm_build/./bin/clang  --driver-mode=g++ -gline-tables-only -fsanitize=thread  -funwind-tables --sysroot=/b/s/w/ir/x/w/cipd/linux  -I/b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/test -ldl /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/abort_on_error.cpp -o /b/s/w/ir/x/w/llvm_build/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/compiler-rt/test/sanitizer_common/tsan-x86_64-Linux/Linux/Output/abort_on_error.cpp.tmp
+ /b/s/w/ir/x/w/llvm_build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=thread -funwind-tables --sysroot=/b/s/w/ir/x/w/cipd/linux -I/b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/test -ldl /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/abort_on_error.cpp -o /b/s/w/ir/x/w/llvm_build/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/compiler-rt/test/sanitizer_common/tsan-x86_64-Linux/Linux/Output/abort_on_error.cpp.tmp
ld.lld: error: undefined symbol: __gcc_personality_v0
>>> referenced by abort_on_error.cpp
>>>               /b/s/w/ir/x/t/lit-tmp-ltnvtf5j/abort_on_error-0ca4b8.o:(DW.ref.__gcc_personality_v0)
clang: error: linker command failed with exit code 1 (use -v to see invocation)

--

********************

https://logs.chromium.org/logs/fuchsia/led/gulfem_google.com/141f7eb5f57d1686c0532c0155e6deca355b20629e48dedea6078c47d6e7a825/+/u/clang/test/stdout

@tstellar
Copy link
Collaborator

Can you revert this. I don't think this was sufficiently reivewed.

@tstellar
Copy link
Collaborator

Reverted in f6ca6ed.

@thesamesam thesamesam reopened this Jan 24, 2024
@thesamesam
Copy link
Member

Thanks.

Copy link
Collaborator

@tstellar tstellar left a comment

Choose a reason for hiding this comment

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

This needs to be reviewed by someone familiar with the compiler-rt build system.

@kwk
Copy link
Contributor

kwk commented Feb 22, 2024

This needs to be reviewed by someone familiar with the compiler-rt build system.

@tstellar is there any progress on this? Without this PR this test fails for us: https://src.fedoraproject.org/tests/compiler-rt/blob/main/f/fp16-abi

To reproduce it locally with a recent clang you can do this:

$ cat <<EOF > /tmp/test.cpp                                           
#include <iostream>
#include <algorithm>
#include <vector>

int main()
{
    _Float16 one_f16 = 0.123;
    float one_f32 = one_f16;
    std::cout <<  one_f32 << std::endl;
    return 0;
}
EOF
$ clang++ --rtlib=compiler-rt -O0 /tmp/test.cpp -o /tmp/test
$ /tmp/test

The expected output is 0.122986 but the actual output is 5.96046e-08. This only happens for -O0 by the way.

For the moment I'm going to cherry-pick this PR and have it applied to our snapshot builds.

overmighty added a commit to overmighty/llvm-project that referenced this pull request Aug 15, 2024
…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 added a commit that referenced this pull request Aug 30, 2024
…ecks (#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 #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.
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)
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)
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants