Page MenuHomePhabricator

use __ARM_FP instead of __VFP_FP__
Needs ReviewPublic

Authored by kelledin on Sep 15 2018, 5:08 PM.

Details

Summary

Fix detection of ARM toolchain FPU support

Event Timeline

kelledin created this revision.Sep 15 2018, 5:08 PM
Herald added subscribers: Restricted Project, llvm-commits, chrib and 3 others. · View Herald Transcript
kelledin added a comment.EditedSep 16 2018, 8:10 AM

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.

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.

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