Skip to content

Commit

Permalink
[ARM] Fix recent breakage of -mfpu=none.
Browse files Browse the repository at this point in the history
The recent change D60691 introduced a bug in clang when handling
option combinations such as `-mcpu=cortex-m4 -mfpu=none`. Those
options together should select Cortex-M4 but disable all use of
hardware FP, but in fact, now hardware FP instructions can still be
generated in that mode.

The reason is because the handling of FPUVersion::NONE disables all
the same feature names it used to, of which the base one is `vfp2`.
But now there are further features below that, like `vfp2d16fp` and
(following D60694) `fpregs`, which also need to be turned off to
disable hardware FP completely.

Added a tiny test which double-checks that compiling a simple FP
function doesn't access the FP registers.

Reviewers: SjoerdMeijer, dmgreen

Reviewed By: dmgreen

Subscribers: lebedev.ri, javed.absar, kristof.beyls, hiraditya, cfe-commits, llvm-commits

Tags: #clang, #llvm

Differential Revision: https://reviews.llvm.org/D62729

llvm-svn: 362380
  • Loading branch information
statham-arm committed Jun 3, 2019
1 parent d8d3e17 commit dc83a3c
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 2 deletions.
9 changes: 7 additions & 2 deletions clang/lib/Driver/ToolChains/Arch/ARM.cpp
Expand Up @@ -430,15 +430,20 @@ void arm::getARMTargetFeatures(const ToolChain &TC,
llvm::ARM::getFPUFeatures(llvm::ARM::FK_NONE, Features);

// Disable hardware FP features which have been enabled.
// FIXME: Disabling vfp2 and neon should be enough as all the other
// features are dependent on these 2 features in LLVM. However
// FIXME: Disabling fpregs should be enough all by itself, since all
// the other FP features are dependent on it. However
// there is currently no easy way to test this in clang, so for
// now just be explicit and disable all known dependent features
// as well.
for (std::string Feature : {"vfp2", "vfp3", "vfp4", "fp-armv8", "fullfp16",
"neon", "crypto", "dotprod", "fp16fml"})
if (std::find(std::begin(Features), std::end(Features), "+" + Feature) != std::end(Features))
Features.push_back(Args.MakeArgString("-" + Feature));

// Disable the base feature unconditionally, even if it was not
// explicitly in the features list (e.g. if we had +vfp3, which
// implies it).
Features.push_back("-fpregs");
}

// En/disable crc code generation.
Expand Down
8 changes: 8 additions & 0 deletions clang/test/CodeGen/arm-mfpu-none.c
@@ -0,0 +1,8 @@
// REQUIRES: arm-registered-target
// RUN: %clang -target arm-none-eabi -mcpu=cortex-m4 -mfpu=none -S -o - %s | FileCheck %s

// CHECK-LABEL: compute
// CHECK-NOT: {{s[0-9]}}
float compute(float a, float b) {
return (a+b) * (a-b);
}
2 changes: 2 additions & 0 deletions clang/test/Driver/arm-mfpu.c
Expand Up @@ -318,6 +318,7 @@
// RUN: | FileCheck --check-prefix=CHECK-NO-FP %s
// CHECK-NO-FP-NOT: "-target-feature" "+soft-float"
// CHECK-NO-FP: "-target-feature" "+soft-float-abi"
// CHECK-NO-FP: "-target-feature" "-fpregs"
// CHECK-NO-FP: "-target-feature" "-vfp2"
// CHECK-NO-FP: "-target-feature" "-vfp3"
// CHECK-NO-FP: "-target-feature" "-vfp4"
Expand Down Expand Up @@ -363,6 +364,7 @@
// CHECK-SOFT-ABI-FP: "-target-feature" "-fp-armv8"
// CHECK-SOFT-ABI-FP: "-target-feature" "-neon"
// CHECK-SOFT-ABI-FP: "-target-feature" "-crypto"
// CHECK-SOFT-ABI-FP: "-target-feature" "-fpregs"

// RUN: %clang -target arm-linux-androideabi21 %s -### -c 2>&1 \
// RUN: | FileCheck --check-prefix=CHECK-ARM5-ANDROID-FP-DEFAULT %s
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/Support/ARMTargetParser.cpp
Expand Up @@ -198,6 +198,7 @@ bool ARM::getFPUFeatures(unsigned FPUKind, std::vector<StringRef> &Features) {
Features.push_back("-fp-armv8");
break;
case FPUVersion::NONE:
Features.push_back("-fpregs");
Features.push_back("-vfp2");
Features.push_back("-vfp3");
Features.push_back("-fp16");
Expand Down

0 comments on commit dc83a3c

Please sign in to comment.