[ARM] Fix recent breakage of -mfpu=none.

[simon_tatham]

Summary

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.

[lebedev.ri]

clang/test/CodeGen/arm-mfpu-none.c

2 ↗

(On Diff #202413)

Generally clang codegen tests should test -emit-llvm -S output

[dmgreen]

clang/test/CodeGen/arm-mfpu-none.c

2 ↗

(On Diff #202413)

Perhaps add to the tests in clang/test/Driver/arm-mfpu.c instead?

[simon_tatham]

clang/test/CodeGen/arm-mfpu-none.c

2 ↗

(On Diff #202413)

@lebedev.ri : looking at the IR doesn't show up what I'm trying to test in this case. And I modelled this on existing tests in test/CodeGen, such as arm-eabi.c, which also need to test the real assembler output in order to do their job.

@dmgreen: it's true that I could also make test/Driver/arm-mfpu.c check that the extra feature name is appearing in the cc1 command line where I currently think it should be, but I think that wouldn't replace this test. The real point of this test is that the currently right feature names might not stay right, if the feature set is reorganised again in future – and if that happens, then whoever does it will probably rewrite most of arm-mfpu.c anyway.

So this is an end-to-end check that whatever detailed command line is generated by the driver in these circumstances, it is sufficient to ensure that the final generated code really doesn't include FP instructions.

[dmgreen]

clang/test/CodeGen/arm-mfpu-none.c

2 ↗

(On Diff #202413)

I believe that the idea is that if we test each step, so make sure we have tests from clang driver -> clang-cl and clang -> llvm-ir and llvm -> asm, then it should all be tested. And because each has a well defined interfaces along each step of the way it should keep working. Any integration tests are often left to external tests like the test suite.

But I do see some precedent for this in other places. If you add the extra checks in test/Driver/arm-mfpu.c, then I think this is fine. (And I guess if someone objects we can always take it out :) )

[simon_tatham]

clang/test/CodeGen/arm-mfpu-none.c

2 ↗

(On Diff #202413)

I think the shortcomings of that step-by-step testing strategy are exactly what caused the breakage in the first place.

If you know you're changing the mapping from driver arguments to cc1 feature options, then you change the test that obviously goes with the change you just made; but you leave unchanged the test that says the previous set of cc1 options caused the behaviour you wanted – because nothing will remind you that that needs changing as well (or make you aware that that test even exists to be updated, if you didn't already know).

An end-to-end test that ensures that this user input continues to cause that ultimate output code, regardless of the intervening details, would have prevented this bug arising last week, where the other strategy didn't. (Indeed, an end-to-end test of exactly that kind in our downstream tests is how we spotted it shortly after the change landed.)

So I still think I'd prefer not to remove this end-to-end test (though I will if people absolutely insist). But I'll add those extra checks you mention in arm-mfpu.c.

[simon_tatham]

Added the extra checks in arm-mfpu.c.

[dmgreen]

LGTM. Thanks for adding the new test. I agree with keeping both tests ( I wasn't very clear).

cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp

fp16_fml_fallthrough:
 
   // Setting -msoft-float/-mfloat-abi=soft effectively disables the FPU (GCC
   // ignores the -mfpu options in this case).
   // Note that the ABI can also be set implicitly by the target selected.
   if (ABI == arm::FloatABI::Soft) {
     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.
   if (Arg *A = Args.getLastArg(options::OPT_mcrc, options::OPT_mnocrc)) {
     if (A->getOption().matches(options::OPT_mcrc))
       Features.push_back("+crc");
     else
       Features.push_back("-crc");

cfe/trunk/test/CodeGen/arm-mfpu-none.c

+// 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);
+}

cfe/trunk/test/Driver/arm-mfpu.c

 // CHECK-CRYPTO-NEON-FP-ARMV8-NOT: "-target-feature" "+soft-float-abi"
 // CHECK-CRYPTO-NEON-FP-ARMV8: "-target-feature" "+fp-armv8"
 // CHECK-CRYPTO-NEON-FP-ARMV8: "-target-feature" "+crypto"
 
 // RUN: %clang -target armv8-linux-gnueabi -mfpu=none %s -### 2>&1 \
 // 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"
 // CHECK-NO-FP: "-target-feature" "-fp-armv8"
 // CHECK-NO-FP-NOT: "-target-feature" "+fp64"
 // CHECK-NO-FP-NOT: "-target-feature" "+d32"
 // CHECK-NO-FP: "-target-feature" "-neon"
 // CHECK-NO-FP: "-target-feature" "-crypto"

 // CHECK-SOFT-ABI-FP: "-target-feature" "+soft-float"
 // CHECK-SOFT-ABI-FP: "-target-feature" "+soft-float-abi"
 // CHECK-SOFT-ABI-FP: "-target-feature" "-vfp2"
 // CHECK-SOFT-ABI-FP: "-target-feature" "-vfp3"
 // CHECK-SOFT-ABI-FP: "-target-feature" "-vfp4"
 // 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
 // CHECK-ARM5-ANDROID-FP-DEFAULT: "-target-feature" "+soft-float"
 // CHECK-ARM5-ANDROID-FP-DEFAULT: "-target-feature" "+soft-float-abi"
 // CHECK-ARM5-ANDROID-FP-DEFAULT-NOT: "-target-feature" "+d32"
 // CHECK-ARM5-ANDROID-FP-DEFAULT-NOT: "-target-feature" "+vfp3"
 // CHECK-ARM5-ANDROID-FP-DEFAULT-NOT: "-target-feature" "+vfp4"

llvm/trunk/lib/Support/ARMTargetParser.cpp

bool ARM::getFPUFeatures(unsigned FPUKind, std::vector<StringRef> &Features) {
   case FPUVersion::VFPV2:
     Features.push_back("+vfp2");
     Features.push_back("-vfp3");
     Features.push_back("-fp16");
     Features.push_back("-vfp4");
     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");
     Features.push_back("-vfp4");
     Features.push_back("-fp-armv8");
     break;
   }