This is an archive of the discontinued LLVM Phabricator instance.

Differential D47217

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

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

Details

Reviewers
rengolin
samsonov
compnerd
smeenai
javed.absar
peter.smith
Commits
rGec6bdf8d4f5c: [cmake] [ARM] Exclude any VFP builtins if VFP is not supported
rG80968a9aba75: [cmake] [ARM] Check if VFP is supported before including any VFP builtins
rCRT338284: [cmake] [ARM] Exclude any VFP builtins if VFP is not supported
rL338284: [cmake] [ARM] Exclude any VFP builtins if VFP is not supported
rCRT333216: [cmake] [ARM] Check if VFP is supported before including any VFP builtins
rL333216: [cmake] [ARM] Check if VFP is supported before including any VFP builtins
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
Herald added a reviewer: javed.absar. · View Herald TranscriptMay 22 2018, 12:35 PM
Herald added subscribers: chrib, kristof.beyls, mgorny. · View Herald Transcript
peter.smith added a subscriber: peter.smith.May 23 2018, 3:59 AM

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

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
Closed by commit rL333216: [cmake] [ARM] Check if VFP is supported before including any VFP builtins (authored by azharudd). · Explain WhyMay 24 2018, 11:57 AM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: delcypher. · View Herald TranscriptMay 24 2018, 11:57 AM
azharudd added a comment.May 24 2018, 11:58 AM

@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
peter.smith added a comment.Jul 17 2018, 9:52 AM

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.

azharudd added a comment.Jul 23 2018, 5:58 PM
In D47217#1165279, @peter.smith wrote:

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.

azharudd added a comment.Jul 27 2018, 9:53 AM

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
azharudd added a comment.Jul 30 2018, 11:17 AM

@peter.smith Thanks for the review.

Closed by commit rL338284: [cmake] [ARM] Exclude any VFP builtins if VFP is not supported (authored by azharudd). · Explain WhyJul 30 2018, 11:19 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
cmake/
6 lines
lib/
builtins/
19 lines

Diff 148071

cmake/builtin-config-ix.cmake

Show First 20 Lines • Show All 41 Lines • ▼ Show 20 Lines
endif() endif()
set(ALL_BUILTIN_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} set(ALL_BUILTIN_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64}
${MIPS32} ${MIPS64} ${PPC64} ${RISCV32} ${RISCV64} ${WASM32} ${WASM64}) ${MIPS32} ${MIPS64} ${PPC64} ${RISCV32} ${RISCV64} ${WASM32} ${WASM64})
include(CompilerRTUtils) include(CompilerRTUtils)
include(CompilerRTDarwinUtils) include(CompilerRTDarwinUtils)
# If targeting ARM, check if VFP is supported.
if(CAN_TARGET_arm)
string(REPLACE ";" " " _TARGET_arm_CFLAGS "${TARGET_arm_CFLAGS}")
check_compile_definition(__VFP_FP__ "${CMAKE_C_FLAGS} ${_TARGET_arm_CFLAGS}" COMPILER_RT_HAS_ARM_VFP)
endif()
if(APPLE) if(APPLE)
find_darwin_sdk_dir(DARWIN_osx_SYSROOT macosx) find_darwin_sdk_dir(DARWIN_osx_SYSROOT macosx)
find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator) find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator)
find_darwin_sdk_dir(DARWIN_ios_SYSROOT iphoneos) find_darwin_sdk_dir(DARWIN_ios_SYSROOT iphoneos)
find_darwin_sdk_dir(DARWIN_watchossim_SYSROOT watchsimulator) find_darwin_sdk_dir(DARWIN_watchossim_SYSROOT watchsimulator)
find_darwin_sdk_dir(DARWIN_watchos_SYSROOT watchos) find_darwin_sdk_dir(DARWIN_watchos_SYSROOT watchos)
find_darwin_sdk_dir(DARWIN_tvossim_SYSROOT appletvsimulator) find_darwin_sdk_dir(DARWIN_tvossim_SYSROOT appletvsimulator)
▲ Show 20 Lines • Show All 123 Lines • Show Last 20 Lines

lib/builtins/CMakeLists.txt

Show First 20 Lines • Show All 388 Lines • ▼ Show 20 Linesset(arm_Thumb1_VFPv2_SOURCES
arm/nesf2vfp.S arm/nesf2vfp.S
arm/subdf3vfp.S arm/subdf3vfp.S
arm/subsf3vfp.S arm/subsf3vfp.S
arm/truncdfsf2vfp.S arm/truncdfsf2vfp.S
arm/unorddf2vfp.S arm/unorddf2vfp.S
arm/unordsf2vfp.S) arm/unordsf2vfp.S)
set(arm_Thumb1_icache_SOURCES set(arm_Thumb1_icache_SOURCES
arm/sync_synchronize.S) arm/sync_synchronize.S)
# If VFP is supported, include arm_Thumb1_SjLj_EH_SOURCES and
# arm_Thumb1_VFPv2_SOURCES in arm_Thumb1_SOURCES.
if(NOT COMPILER_RT_HAS_ARM_VFP)
peter.smithUnsubmitted
Done
ReplyInline Actions

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()
peter.smith: Would it be worth reordering so that we don't stat the test with a NOT. For example: ``` if…
set(arm_Thumb1_SOURCES
${arm_Thumb1_JT_SOURCES}
${arm_Thumb1_icache_SOURCES})
else()
set(arm_Thumb1_SOURCES set(arm_Thumb1_SOURCES
${arm_Thumb1_JT_SOURCES} ${arm_Thumb1_JT_SOURCES}
${arm_Thumb1_SjLj_EH_SOURCES} ${arm_Thumb1_SjLj_EH_SOURCES}
${arm_Thumb1_VFPv2_SOURCES} ${arm_Thumb1_VFPv2_SOURCES}
${arm_Thumb1_icache_SOURCES}) ${arm_Thumb1_icache_SOURCES})
endif()
if(MINGW) if(MINGW)
set(arm_SOURCES set(arm_SOURCES
arm/aeabi_idivmod.S arm/aeabi_idivmod.S
arm/aeabi_ldivmod.S arm/aeabi_ldivmod.S
arm/aeabi_uidivmod.S arm/aeabi_uidivmod.S
arm/aeabi_uldivmod.S arm/aeabi_uldivmod.S
divmoddi4.c divmoddi4.c
▲ Show 20 Lines • Show All 159 Lines • Show Last 20 Lines