[builtins] Only build float16/bfloat16 code if actually supported

[arichardson]

Summary

When building compiler-rt builtins for x86_64 they library will by default
also be built for i386. We unconditionally add the Float16 compile flags
since the check for Float16 support will be done using x86_64 compiler
flags, but i386 does not actually support it. Fix this by moving the
COMPILER_RT_HAS_FLOAT16 check to a per-target-architecture check.

Many of the checks in the builtin-config-ix file should probably also be
changed to per-target-arch checks, but so far only the Float16 one has
caused issues. This is an alternative to D136044 which added a special case
for i386 FreeBSD.

Fixes: https://github.com/llvm/llvm-project/issues/57224

[arichardson]

compiler-rt/lib/builtins/CMakeLists.txt

787

[dim]

Note that the original problem was not only about _Float16 (COMPILER_RT_HAS_FLOAT16 in CMake), but also __bf16 (COMPILER_RT_HAS_BFLOAT16 in CMake). Both failed on i386-freebsd.

[pengfei]

Thanks for the fix, but I may not be qualified to approve.

When building compiler-rt builtins for x86_64 they library will by default

they -> the

[arichardson]

Also handle bfloat16 (didn't notice this before since I was building with clang 14)

[arichardson]

compiler-rt/lib/builtins/CMakeLists.txt

815

@pengfei Is there a reason bfloat16 is disabled for apple platforms, or was it the same x86_64/i386 problem that this patch fixes?

[pengfei]

compiler-rt/lib/builtins/CMakeLists.txt

815

I guess it is used to solve a build bot fail that cross compiling armv7 over x86_64, see comments on that patch. It might be the same reason as i386, but I didn't investigate it.

[dim]

compiler-rt/lib/builtins/CMakeLists.txt

815

Yes, that looks like D131147#3711517, where it probably detected bf16 support during the configure phase, when it was targeting aarch64. But then it used the same flags to compile armv7, where bf16 doesn't work. This is very similar to the x86_64/i386 situation, where x86_64 does support bf16, but i386 does not.

I _hope_ that arichardson's fix will make this work in a more correct fashion, i.e. do the detection again for each separate target. That would the issue for both aarch64/armv7 and x86_64/i386. (And maybe there are other such combinations...)

[arichardson]

Fix bfloat16 case

[arichardson]

Drop APPLE special case

[arichardson]

fix typo

[dim]

Yes, this looks good to me. Tried with very recent main:

...
-- Performing Test COMPILER_RT_HAS_ASM_LSE
-- Performing Test COMPILER_RT_HAS_ASM_LSE - Failed
-- Looking for __i386__
-- Looking for __i386__ - found
-- Builtin supported architectures: i386;x86_64
-- Performing additional configure checks with target flags: -m32
-- Performing Test COMPILER_RT_HAS_i386_FLOAT16
-- Performing Test COMPILER_RT_HAS_i386_FLOAT16 - Failed
-- Performing Test COMPILER_RT_HAS_i386_BFLOAT16
-- Performing Test COMPILER_RT_HAS_i386_BFLOAT16 - Failed
-- For i386 builtins preferring i386/fp_mode.c to fp_mode.c
-- For i386 builtins preferring i386/ashldi3.S to ashldi3.c
...
-- Performing additional configure checks with target flags: -m64
-- Performing Test COMPILER_RT_HAS_x86_64_FLOAT16
-- Performing Test COMPILER_RT_HAS_x86_64_FLOAT16 - Success
-- Performing Test COMPILER_RT_HAS_x86_64_BFLOAT16
-- Performing Test COMPILER_RT_HAS_x86_64_BFLOAT16 - Success
-- For x86_64 builtins preferring i386/fp_mode.c to fp_mode.c
-- For x86_64 builtins preferring x86_64/floatdidf.c to floatdidf.c
...

So indeed it detects i386 (with -m32) and x86_64 (with -m64) separately, and then both runtimes are built successfully:

[164/304] Linking C static library /home/dim/obj/llvmorg-17-init-4107-g439eebab81c3-freebsd14-amd64-ninja-clang-rel-1/lib/clang/17/lib/i386-unknown-freebsd14.0/libclang_rt.builtins.a
...
[304/304] Linking C static library /home/dim/obj/llvmorg-17-init-4107-g439eebab81c3-freebsd14-amd64-ninja-clang-rel-1/lib/clang/17/lib/x86_64-unknown-freebsd14.0/libclang_rt.builtins.a

[jrtc27]

compiler-rt/cmake/builtin-config-ix.cmake

15

I could believe this one could be a problem for some architectures. The others are probably fine... but I do wonder whether all of this should be made target-specific just to be safe. They're not causing immediate known issues though I guess.

compiler-rt/lib/builtins/CMakeLists.txt

786

Commit message should explain this change (not that it's hard to figure out)

813

Wonky indentation

[arichardson]

compiler-rt/cmake/builtin-config-ix.cmake

15

IMO we should just make all checks target dependent but in this patch I wanted to focus on the currently known broken cases before refactoring all the checks.

[arichardson]

Will fix the outstanding issues and commit tomorrow. Thanks for reviewing and testing.

[chapuni]

compiler-rt/lib/builtins/CMakeLists.txt

809

CMake's check_c_source_compiles() would fail if main() is not provided. Is it intentional?

In runtime builds, CMAKE_C_COMPILER_WORKS=ON hides the problem, though.

[dim]

compiler-rt/lib/builtins/CMakeLists.txt

809

You may be right, it looks as if the assumption is (not only in this case) that check_c_source_compiles() just checks if it compiles, not if it links. There are a few hand-made versions that do exactly that, but CMake's documentation states:

Check that the source supplied in <code> can be compiled as a C source file and linked as an executable (so it must contain at least a main() function).

So I think quite a lot of these instances might have to be replaced. Why didn't anybody notice this before? I certainly did not, and I think the function name check_c_source_compiles is very misleading...

compiler-rt/cmake/builtin-config-ix.cmake

 include(BuiltinTests)
 include(CheckCSourceCompiles)
 
 # Make all the tests only check the compiler
 set(TEST_COMPILE_ONLY On)
 
 # Check host compiler support for certain flags
 builtin_check_c_compiler_flag(-fPIC                 COMPILER_RT_HAS_FPIC_FLAG)
 builtin_check_c_compiler_flag(-fPIE                 COMPILER_RT_HAS_FPIE_FLAG)
 builtin_check_c_compiler_flag(-fno-builtin          COMPILER_RT_HAS_FNO_BUILTIN_FLAG)
 builtin_check_c_compiler_flag(-std=c11              COMPILER_RT_HAS_STD_C11_FLAG)
 builtin_check_c_compiler_flag(-fvisibility=hidden   COMPILER_RT_HAS_VISIBILITY_HIDDEN_FLAG)
 builtin_check_c_compiler_flag(-fomit-frame-pointer  COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG)
 builtin_check_c_compiler_flag(-ffreestanding        COMPILER_RT_HAS_FFREESTANDING_FLAG)
 builtin_check_c_compiler_flag(-fxray-instrument     COMPILER_RT_HAS_XRAY_COMPILER_FLAG)
 
 builtin_check_c_compiler_source(COMPILER_RT_HAS_ATOMIC_KEYWORD
 "
 int foo(int x, int y) {
  _Atomic int result = x * y;
  return result;
 }
 ")
 
-builtin_check_c_compiler_source(COMPILER_RT_HAS_FLOAT16
-"
-_Float16 foo(_Float16 x) {
- return x;
-}
-"
-)
-
 builtin_check_c_compiler_source(COMPILER_RT_HAS_BFLOAT16
 "
 __bf16 foo(__bf16 x) {
  return x;
 }
 "
 )
 

compiler-rt/lib/builtins/CMakeLists.txt

if (COMPILER_RT_STANDALONE_BUILD)
   # option provides an avenue through which the flag may be controlled when an
   # LLVM configuration is not being utilized.
   option(COMPILER_RT_BUILTINS_ENABLE_PIC
     "Turns on or off -fPIC for the builtin library source"
     ON)
 endif()
 
 include(builtin-config-ix)
+include(CMakePushCheckState)
 
 if(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
   include(CompilerRTAIXUtils)
 endif()
 
 option(COMPILER_RT_BUILTINS_HIDE_SYMBOLS
   "Do not export any symbols from the static library." ON)
 

 else ()
   set(BUILTIN_CFLAGS "")
   add_security_warnings(BUILTIN_CFLAGS 0)
 
   if (COMPILER_RT_HAS_FCF_PROTECTION_FLAG)
     append_list_if(COMPILER_RT_ENABLE_CET -fcf-protection=full BUILTIN_CFLAGS)
   endif()
 
-  append_list_if(COMPILER_RT_HAS_FLOAT16 -DCOMPILER_RT_HAS_FLOAT16 BUILTIN_CFLAGS)
-
   append_list_if(COMPILER_RT_HAS_STD_C11_FLAG -std=c11 BUILTIN_CFLAGS)
 
   # These flags would normally be added to CMAKE_C_FLAGS by the llvm
   # cmake step. Add them manually if this is a standalone build.
   if(COMPILER_RT_STANDALONE_BUILD)
     if(COMPILER_RT_BUILTINS_ENABLE_PIC)
       append_list_if(COMPILER_RT_HAS_FPIC_FLAG -fPIC BUILTIN_CFLAGS)
     endif()
else ()
   if(COMPILER_RT_DISABLE_AARCH64_FMV)
     list(APPEND BUILTIN_DEFS DISABLE_AARCH64_FMV)
   endif()
 
   append_list_if(COMPILER_RT_HAS_ASM_LSE HAS_ASM_LSE BUILTIN_DEFS)
 
   foreach (arch ${BUILTIN_SUPPORTED_ARCH})
     if (CAN_TARGET_${arch})
+      cmake_push_check_state()
+      # TODO: we should probably most more of the checks in builtin-config depend on the target flags.
+      message(STATUS "Performing additional configure checks with target flags: ${TARGET_${arch}_CFLAGS}")
       set(BUILTIN_CFLAGS_${arch} ${BUILTIN_CFLAGS})
+      list(APPEND CMAKE_REQUIRED_FLAGS ${TARGET_${arch}_CFLAGS} ${BUILTIN_CFLAGS_${arch}})
       # For ARM archs, exclude any VFP builtins if VFP is not supported
       if (${arch} MATCHES "^(arm|armhf|armv7|armv7s|armv7k|armv7m|armv7em|armv8m.main|armv8.1m.main)$")
         string(REPLACE ";" " " _TARGET_${arch}_CFLAGS "${TARGET_${arch}_CFLAGS}")
         check_compile_definition(__ARM_FP "${CMAKE_C_FLAGS} ${_TARGET_${arch}_CFLAGS}" COMPILER_RT_HAS_${arch}_VFP)
         if(NOT COMPILER_RT_HAS_${arch}_VFP)
           list(REMOVE_ITEM ${arch}_SOURCES ${arm_Thumb1_VFPv2_DP_SOURCES} ${arm_Thumb1_VFPv2_SP_SOURCES} ${arm_Thumb1_SjLj_EH_SOURCES})
         else()
           # Exclude any double-precision builtins if VFP is single-precision-only
           try_compile_only(COMPILER_RT_HAS_${arch}_VFP_DP
                            SOURCE "#if !(__ARM_FP & 0x8)
                                    #error No double-precision support!
                                    #endif
                                    int main(void) { return 0; }")
           if(NOT COMPILER_RT_HAS_${arch}_VFP_DP)
             list(REMOVE_ITEM ${arch}_SOURCES ${arm_Thumb1_VFPv2_DP_SOURCES})
           endif()
         endif()
       endif()
+      check_c_source_compiles("_Float16 foo(_Float16 x) { return x; }"
+                              COMPILER_RT_HAS_${arch}_FLOAT16)
+      append_list_if(COMPILER_RT_HAS_${arch}_FLOAT16 -DCOMPILER_RT_HAS_FLOAT16 BUILTIN_CFLAGS_${arch})
 
       # Remove a generic C builtin when an arch-specific builtin is specified.
       filter_builtin_sources(${arch}_SOURCES ${arch})
 
       # Needed for clear_cache on debug mode, due to r7's usage in inline asm.
       # Release mode already sets it via -O2/3, Debug mode doesn't.
       if (${arch} STREQUAL "armhf")
         list(APPEND BUILTIN_CFLAGS_${arch} -fomit-frame-pointer -DCOMPILER_RT_ARMHF_TARGET)
       endif()
 
       # For RISCV32, we must force enable int128 for compiling long
       # double routines.
if (CAN_TARGET_${arch})
       add_compiler_rt_runtime(clang_rt.builtins
                               STATIC
                               ARCHS ${arch}
                               DEPS ${deps_${arch}}
                               SOURCES ${${arch}_SOURCES}
                               DEFS ${BUILTIN_DEFS}
                               CFLAGS ${BUILTIN_CFLAGS_${arch}}
                               PARENT_TARGET builtins)
+      cmake_pop_check_state()
     endif ()
   endforeach ()
 endif ()
 
 option(COMPILER_RT_BUILD_STANDALONE_LIBATOMIC
   "Build standalone shared atomic library."
   OFF)