use __ARM_FP instead of __VFP_FP__

[kelledin]

Summary

Fix detection of ARM toolchain FPU support

[kelledin]

BTW, I also see a bunch of __VFP_FP__ checks in compiler-rt/test/builtins/Unit/*_test.c . Somehow tests have still passed in my changeset with this change added, but with the *_test.c files still using __VFP_FP__. Should we still tweak those to use __ARM_FP instead?

My feeling is that if we're going to put that kind of check in the source code, we might as well put in the correct check.

[peter.smith]

I've checked with the gcc sources and it is VFP_FP is unconditionally set by gcc. This might be a relatively recent change so I suspect that clang will have been following early behaviour of gcc. It will be worth updating the tests to use __ARM_FP as well. It will also be worth working out why the tests didn't fail for you as well. I'd only expect to see a difference in behaviour if you were using gcc without floating point support though.

lib/builtins/CMakeLists.txt

571

I suggest rewording the comment a bit. Once VFP_FP has been removed then I don't think it is worth commenting about not using it. I think the review summary/source code commit message is the best place to mention that.

I think that your #TODO is correct. As it stands we can't compile compiler-rt if a cpu with a single precision only floating point unit is used. I note that the cmake recipe in clang/cmake/caches/BaremetalARM.cmake cheats and uses -mfpu=fp-armv8 which will allow the double precision files to compile. This is not ideal but the uses of double precision in assembly language seem to be restricted to Darwin, which I don't think are ever called from a single precision only target. It is possible in the future that architecturally illegal combinations are forbidden so this is something we should fix.

My suggestion for the comment is:
FIXME: We do not split out the single-precision and double-precision VFPv2 sources. This means that compiler-rt will not build if we specify a single precision only FPU. We may be able to use the value of __ARM_FP to remove the double precision sources if double precision is not available.

[compnerd]

Wow, thanks for fixing this. I believe that @peter.smith is right and that the behaviour is something new.

lib/builtins/CMakeLists.txt

if (CAN_TARGET_${arch})
       set(_arch ${arch})
       if("${arch}" STREQUAL "armv6m")
         set(_arch "arm|armv6m")
       elseif("${arch}" MATCHES "^(armhf|armv7|armv7s|armv7k|armv7m|armv7em)$")
         set(_arch "arm")
       endif()
 
       # For ARM archs, exclude any VFP builtins if VFP is not supported
+      # NOTE: __VFP_FP__ is unconditionally defined by gcc regardless of build
+      # flags, so it's not a good indicator to rely on.  Use __ARM_FP instead.
+      # TODO: *maybe* detect FPU feature level from __ARM_FP?  I would expect
+      # we need both single and double precision FP support, which means we
+      # need at least bits 2 and 3 set in __ARM_FP.
       if (${arch} MATCHES "^(arm|armhf|armv7|armv7s|armv7k|armv7m|armv7em)$")
         string(REPLACE ";" " " _TARGET_${arch}_CFLAGS "${TARGET_${arch}_CFLAGS}")
-        check_compile_definition(__VFP_FP__ "${CMAKE_C_FLAGS} ${_TARGET_${arch}_CFLAGS}" COMPILER_RT_HAS_${arch}_VFP)
+        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_SOURCES} ${arm_Thumb1_SjLj_EH_SOURCES})
         endif()
       endif()
 
       # Filter out generic versions of routines that are re-implemented in
       # architecture specific manner.  This prevents multiple definitions of the
       # same symbols, making the symbol selection non-deterministic.