Page MenuHomePhabricator

[cmake] [ARM] Exclude any VFP builtins if VFP is not supported
ClosedPublic

Authored by azharudd on May 22 2018, 12:35 PM.

Details

Summary

rL325492 disables FPU features when using soft floating point
(-mfloat-abi=soft), which is used internally when building for arm. This causes
errors with builtins that utililize VFP instructions.

With this change we check if VFP is enabled (by checking if the preprocessor
macro VFP_FP is defined), and exclude such builtins if it is not enabled.

Diff Detail

Repository
rL LLVM

Event Timeline

azharudd created this revision.May 22 2018, 12:35 PM

I think that this is a sensible thing to do and it also helps out when trying to build compiler-rt for armv7-m as compiler-rt currently assumes that you have a target with a VFP when many armv7-m devices do not.

lib/builtins/CMakeLists.txt
400 ↗(On Diff #148071)

Would it be worth reordering so that we don't stat the test with a NOT. For example:

if (COMPILER_RT_HAS_ARM_VFP)
  set(arm_Thumb1_SOURCES
    ${arm_Thumb1_JT_SOURCES}
    ${arm_Thumb1_SjLj_EH_SOURCES}
    ${arm_Thumb1_VFPv2_SOURCES}
    ${arm_Thumb1_icache_SOURCES})
else()
  set(arm_Thumb1_SOURCES
  ${arm_Thumb1_JT_SOURCES}
  ${arm_Thumb1_icache_SOURCES})
endif()
azharudd updated this revision to Diff 148270.May 23 2018, 12:45 PM
azharudd added a reviewer: peter.smith.
azharudd marked an inline comment as done.
peter.smith accepted this revision.May 24 2018, 2:38 AM

Thanks for the update. That looks good to me.

This revision is now accepted and ready to land.May 24 2018, 2:38 AM
This revision was automatically updated to reflect the committed changes.

@peter.smith Thanks for the review.

azharudd reopened this revision.Jul 16 2018, 11:57 AM
This revision is now accepted and ready to land.Jul 16 2018, 11:57 AM
azharudd retitled this revision from [cmake] [ARM] Check if VFP is supported before including any VFP builtins to [cmake] [ARM] Exclude any VFP builtins if VFP is not supported.Jul 16 2018, 11:58 AM
azharudd edited the summary of this revision. (Show Details)
azharudd updated this revision to Diff 155733.Jul 16 2018, 12:02 PM

I had to revert this last time as it was causing the armhf buildbot to fail. This should take care of that.

azharudd requested review of this revision.Jul 16 2018, 12:03 PM

Can you elaborate on why the armhf buildbot failed and why you have excluded armhf from the MATCHES expression above. A simple test of a source file containing

__VFP_FP__

clang --target=arm-linux-gnueabihf -E vfp.c gives me

# 1 "vfp.c"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 361 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "vfp.c" 2
1

Unless I'm missing something a target of arm-linux-gnueabihf would define the macro unless the fpu were disabled with -mfpu=none so it should come through the test with the right answer.

Can you elaborate on why the armhf buildbot failed and why you have excluded armhf from the MATCHES expression above. A simple test of a source file containing

__VFP_FP__

clang --target=arm-linux-gnueabihf -E vfp.c gives me

# 1 "vfp.c"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 361 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "vfp.c" 2
1

Unless I'm missing something a target of arm-linux-gnueabihf would define the macro unless the fpu were disabled with -mfpu=none so it should come through the test with the right answer.

Regarding the buildbot failure: the "armhf" sources list is populated from the "arm" sources list, and is the same as that. In the previous change, the VFP builtin sources were being excluded from the "arm" sources list earlier, resulting in them being excluded from the following "armhf" sources list as well. This was causing the tests related to those VFP sources to fail on the "armhf" builder.

why you have excluded armhf from the MATCHES expression above

It shouldn't actually be excluded. When writing that if condition I assumed the VFP sources can always be included for armhf, which is wrong, as yes one can still disable fpu through -mfpu=none as part of the flags. I'll update the patch.

azharudd updated this revision to Diff 156948.Jul 23 2018, 5:59 PM

Updated to check if VFP is enabled for armhf too.

Ping for review please.

peter.smith accepted this revision.Jul 30 2018, 5:44 AM

Apologies for the delay. This looks good to me. I've checked that I can build v7-m without floating point support and I've checked that I can build and run the tests for armv7-linux-gnueabihf.

This revision is now accepted and ready to land.Jul 30 2018, 5:44 AM

@peter.smith Thanks for the review.

This revision was automatically updated to reflect the committed changes.