[ARM][Driver] Warn if -mhard-float is incompatible

[michaelplatings]

Summary

Mixing -mfloat-abi=hard with a CPU that doesn't have floating point
registers is an error in GCC:

cc1: error: '-mfloat-abi=hard': selected processor lacks an FPU

Since there is code in the wild (including in clang tests) that relies
on Clang's current behaviour, emit a warning instead of an error.

Unlike the GCC error, the new warning refers to floating point
registers instead of an FPU. This is because -mfloat-abi=hard and
-march=armv8.1-m.main+mve+nofp are compatible - in that case floating
point registers are required, but an FPU is not required.

My initial thought was to use the floating point ABI calculated by
arm::getARMFloatABI() but in invalid cases which error for other
reasons the ABI is miscalculated and the warning would cause confusion.
Therefore only warn if the user specifies the float ABI explicitly.

Fixes part of https://github.com/llvm/llvm-project/issues/55755

[michaelplatings]

Tweak tests to use -### to run faster.

[simon_tatham]

clang/lib/Driver/ToolChains/Arch/ARM.cpp

151

This whole patch hinges on the unspoken assumption that this is a correct way to test for the absence of FP registers, i.e. that "-fpregs" will always appear in Features if there are no FP registers.

Could you add a comment stating that assumption explicitly and explaining why it's valid? In particular, why you're confident that the absence of FP registers might not sometimes be signalled by the lack of either "-fpregs" or "+fpregs" in that list.

(In a quick look at the code I think you're right, but I'm not 100% confident, and if you show your working then people will be able to work out what went wrong later if things change.)

[michaelplatings]

Add variable specifically for the purpose of specifying whether or not FP regs are available.

[michaelplatings]

clang/lib/Driver/ToolChains/Arch/ARM.cpp

151

That code was relying on implementation details of the function that called it. I've now changed checkARMFloatABI to require that the caller specifies whether or not FP regs are available, and made that calculation clear in the calling function.

[michaelplatings]

Restore tweak to tests to use -### to run faster.

[simon_tatham]

Thanks, that's much clearer. LGTM.

[TWeaver]

Good morning from the UK!

I believe this change has caused the following buildbot to start failing:

https://lab.llvm.org/buildbot/#/builders/139/builds/42214

Is anyone able to take a look?

Much obliged,
Tom W

[michaelplatings]

Thanks Tom, I'll work on it now.

[michaelplatings]

Reverted.

[TWeaver]

Thanks for the speedy reply and action!

[michaelplatings]

Relanded with the warning in the "OptionIgnored" group which fixes the test failure.

[MaskRay]

clang/test/Driver/arm-no-float-regs.c

23

For future changes like this, consider preferring NOWARN-NOT: warning: so that the test is slightly stricter (also assert that no other warning is emitted).

I suspect that // REQUIRES: arm-registered-target is unneeded (test a build that removes ARM from LLVM_TARGETS_TO_BUILD) since you use -### for all clang invocations.

clang/include/clang/Basic/DiagnosticDriverKinds.td

def warn_drv_clang_unsupported : Warning<
   "the clang compiler does not support '%0'">;
 def warn_drv_deprecated_arg : Warning<
   "argument '%0' is deprecated, use '%1' instead">, InGroup<Deprecated>;
 def warn_drv_assuming_mfloat_abi_is : Warning<
   "unknown platform, assuming -mfloat-abi=%0">;
 def warn_drv_unsupported_float_abi_by_lib : Warning<
   "float ABI '%0' is not supported by current library">,
   InGroup<DiagGroup<"unsupported-abi">>;
+def warn_drv_no_floating_point_registers: Warning<
+  "'%0': selected processor lacks floating point registers">;
 def warn_ignoring_ftabstop_value : Warning<
   "ignoring invalid -ftabstop value '%0', using default value %1">;
 def warn_drv_overriding_flag_option : Warning<
   "overriding '%0' option with '%1'">,
   InGroup<DiagGroup<"overriding-t-option">>;
 def warn_drv_treating_input_as_cxx : Warning<
   "treating '%0' input as '%1' when in C++ mode, this behavior is deprecated">,
   InGroup<Deprecated>;

clang/lib/Driver/ToolChains/Arch/ARM.cpp

llvm::ARM::ArchKind ArchKind =
     arm::getLLVMArchKindForARM(CPU, ArchName, Triple);
   if (ArchKind == llvm::ARM::ArchKind::INVALID ||
       (Split.second.size() && !DecodeARMFeatures(D, Split.second, CPU, ArchKind,
                                                  Features, ArgFPUKind)))
     D.Diag(clang::diag::err_drv_unsupported_option_argument)
         << A->getSpelling() << A->getValue();
 }
 
+// If -mfloat-abi=hard or -mhard-float are specified explicitly then check that
+// floating point registers are available on the target CPU.
+static void checkARMFloatABI(const Driver &D, const ArgList &Args,
+                             bool HasFPRegs) {
+  if (HasFPRegs)
+    return;
+  const Arg *A =
+      Args.getLastArg(options::OPT_msoft_float, options::OPT_mhard_float,
+                      options::OPT_mfloat_abi_EQ);
+  if (A && (A->getOption().matches(options::OPT_mhard_float) ||
+            (A->getOption().matches(options::OPT_mfloat_abi_EQ) &&
+             A->getValue() == StringRef("hard"))))
+    D.Diag(clang::diag::warn_drv_no_floating_point_registers)
+        << A->getAsString(Args);
+}
+
 bool arm::useAAPCSForMachO(const llvm::Triple &T) {
   // The backend is hardwired to assume AAPCS for M-class processors, ensure
   // the frontend matches that.
   return T.getEnvironment() == llvm::Triple::EABI ||
          T.getEnvironment() == llvm::Triple::EABIHF ||
          T.getOS() == llvm::Triple::UnknownOS || isARMMProfile(T);
 }
 
fp16_fml_fallthrough:
     else if (ItRNoFullFP16 > ItRFP16FML)
       Features.push_back("+fullfp16");
   }
 
   // Setting -msoft-float/-mfloat-abi=soft, -mfpu=none, or adding +nofp to
   // -march/-mcpu 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.
+  bool HasFPRegs = true;
   if (ABI == arm::FloatABI::Soft) {
     llvm::ARM::getFPUFeatures(llvm::ARM::FK_NONE, Features);
 
     // Disable all features relating to hardware FP, not already disabled by the
     // above call.
-    Features.insert(Features.end(), {"-dotprod", "-fp16fml", "-bf16", "-mve",
-                                     "-mve.fp", "-fpregs"});
+    Features.insert(Features.end(),
+                    {"-dotprod", "-fp16fml", "-bf16", "-mve", "-mve.fp"});
+    HasFPRegs = false;
   } else if (FPUKind == llvm::ARM::FK_NONE ||
              ArchArgFPUKind == llvm::ARM::FK_NONE ||
              CPUArgFPUKind == llvm::ARM::FK_NONE) {
     // -mfpu=none, -march=armvX+nofp or -mcpu=X+nofp is *very* similar to
     // -mfloat-abi=soft, only that it should not disable MVE-I. They disable the
     // FPU, but not the FPU registers, thus MVE-I, which depends only on the
     // latter, is still supported.
     Features.insert(Features.end(),
                     {"-dotprod", "-fp16fml", "-bf16", "-mve.fp"});
-    if (!hasIntegerMVE(Features))
-      Features.emplace_back("-fpregs");
+    HasFPRegs = hasIntegerMVE(Features);
   }
+  if (!HasFPRegs)
+    Features.emplace_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");
   }
if (EnableBlr)
       Features.push_back("+harden-sls-blr");
     if (DisableComdat) {
       Features.push_back("+harden-sls-nocomdat");
     }
   }
 
   if (Args.getLastArg(options::OPT_mno_bti_at_return_twice))
     Features.push_back("+no-bti-at-return-twice");
+
+  checkARMFloatABI(D, Args, HasFPRegs);
 }
 
 std::string arm::getARMArch(StringRef Arch, const llvm::Triple &Triple) {
   std::string MArch;
   if (!Arch.empty())
     MArch = std::string(Arch);
   else
     MArch = std::string(Triple.getArchName());

clang/test/Driver/arm-no-float-regs.c

+// REQUIRES: arm-registered-target
+
+// Check that -mfloat-abi=hard gives a warning if FP registers aren't available.
+// RUN: %clang --target=arm-none-eabi -mcpu=cortex-m0 -mfloat-abi=hard -### -c %s 2>&1
+
+// RUN: %clang --target=arm-none-eabi -mcpu=cortex-m0 -mhard-float -### -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=HARDFLOAT %s
+
+// -mfloat-abi=hard and -march=...+nofp are incompatible in this instance:
+// RUN: %clang --target=arm-none-eabi -march=armv8.1-m.main+nofp -mfloat-abi=hard -### -c %s 2>&1
+// -mfloat-abi=hard and -march=...+nofp are compatible in this instance:
+// RUN: %clang --target=arm-none-eabi -march=armv8.1-m.main+mve+nofp -mfloat-abi=hard -### -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=NOWARN %s
+
+// Here the float ABI is calculated as "hard" and FP registers are
+// calculated to not be available. Since the float ABI wasn't specified
+// explicitly, the warning should not be emitted.
+// RUN: %clang -target thumbv5-windows -mcpu=arm10tdmi -### -c %s -o /dev/null 2>&1 \
+// RUN:   | FileCheck -check-prefix=NOWARN %s
+
+// CHECK: warning: '-mfloat-abi=hard': selected processor lacks floating point registers
+// HARDFLOAT: warning: '-mhard-float': selected processor lacks floating point registers
+// NOWARN-NOT: selected processor lacks floating point registers