[Driver][ARM] Disable unsupported features when nofp arch extension is used

[vhscampos]

Summary

A list of target features is disabled when there is no hardware
floating-point support. This is the case when one of the following
options is passed to clang:

• -mfloat-abi=soft
• -mfpu=none

This option list is missing, however, the extension "+nofp" that can be
specified in -march flags, such as "-march=armv8-a+nofp".

This patch also disables unsupported target features when nofp is passed
to -march.

[chill]

Needs a regression test. This patch and the dependent patch clash, better with a single patch.

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

289

That's kinda mouthful name.

293–298

Wouldn't just looking for the substring do the job?

Also need to handle -mcpu=...+nofp.

We already "parse" the arguments to -march= and -mcpu= (and -mfpu=) earlier, it seems to me we
could note the +nofp and +nofp.dp earlier. (TBH, it isn't immediately obvious to me how to untangle this mess).

[chill]

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

293–298

Hmm, actually, +nofp.dp should not disable the FPU, I think.

[DavidSpickett]

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

297

I would check what this does:
$ ./bin/clang -target arm-arm-none-eabi /tmp/test.c -c -march=armv8.1-a+nofp+fp -###

I'm not sure if by this point we've resolved "+nofp+fp" to simply "+fp". (perhaps you'd end up with a bunch of "-<feature>" followed by the same as "+<feature>". Either way I'd expect +fp to win.

[vhscampos]

I will merge the two patches into one.

Please also see my inline responses.

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

289

Not sure how to compress it more without making it unmeaningful though.

293–298

Just looking for the substring might be sufficient indeed.

Yes, we already do -march/-mcpu parsing a bit earlier. However, this parsing and the following handling of it is done deeper in the call stack. I wondered about ways to propagate this information back to this point here (e.g. adding one more by-ref argument that is set by the first round of parsing), but I don't feel confident to back it up.

Are you okay with me just changing it to a substring search?

[vhscampos]

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

293–298

Actually it may be better to keep the string splitting method. The search required here must be whole-word, as to flag up "+nofp", but not "+nofp.dp". It can be done with less code using the current list of tokens as opposed to using substring search, followed by a "is it whole-word?" check.

297

Good catch. I will be sure to cover this case.

[chill]

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

293–298

In fact, it's less number of tokens, not that number of tokens is that important.

auto anyNOFP = [](const StringRef &str) {
  size_t pos = str.find_lower("+nofp");
  return pos != StringRef::npos &&
          (pos + 5 == str.size() || str[pos + 5] == '+');
};

Or it could become

auto findFeature = [](const StringRef &str, const StringRef &feature) {
  size_t pos = str.find_lower(feature);
  return (pos == StringRef::npos || pos + feature.size() == str.size() ||
          str[pos + feature.size()] == '+')
             ? pos
             : StringRef::npos;
};

which is slightly longer, but more universal and allows you to check relative positions of features.

A-a-a-nyway, I think this string twiddling should be the absolute last resort.

Could you please, see, if checkARMArchName and checkARMCpuName can be tweaked into
returning llvm::ARM::FK_NONE or llvm::ARM::FK_INVALID or whatever, depending on what is present
in option value of -march=... and -mcpu=...?

An interesting question is what to do if there are contradictions, but I think the general strategy is to not strive to produce sensible output from nonsensical input.

[vhscampos]

1. Merged the second patch into this (handle bf16).
2. Do the same treatment for -mcpu.
3. Instead of doing string search once again, return the desired information in the first time using a by-ref argument.
4. This new approach covers positional differences between +fp and +nofp.

[vhscampos]

Ping.

[DavidSpickett]

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

476

Why not disable MVE-I? I assume because it's integer only but then why does -mfloat-abi=soft disable it?

If possible add a regression test for this. In general a test like the bf16 test below, but for all the listed extensions would help. Perhaps it makes more sense to add a driver test that looks for the "-<ext>" bits in the -### output instead of doing each extension on its own.

[chill]

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

476

Why not disable MVE-I?

After MVE, "FPU" registers are a separate entity from the FPU.

-mfpu=none/+nofp disable the FPU. MVE-I does not require an FPU.
-mfloat-abi=soft disables both the FPU instructions and the FPU registers.
MVE-I requires "FPU" registers.

It's possible to define different semantics, but this is the one we agreed with GCC.

[DavidSpickett]

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

476

Got it. @vhscampos can you add that to the comment?

[vhscampos]

1. Add comment explaining the MVE-Integer detail.
2. Add another test to check the disabled features.

[DavidSpickett]

LGTM, I'll let @chill give the final word.

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

476

Thanks, lgtm.

[chill]

LGTM, thanks!

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

482

LLVM coding standards call for not using braces on single-statement bodies and that's also the style in this source file.

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

static unsigned getARMFPUFeatures(const Driver &D, const Arg *A,
                                   std::vector<StringRef> &Features) {
   unsigned FPUID = llvm::ARM::parseFPU(FPU);
   if (!llvm::ARM::getFPUFeatures(FPUID, Features))
     D.Diag(clang::diag::err_drv_clang_unsupported) << A->getAsString(Args);
   return FPUID;
 }
 
 // Decode ARM features from string like +[no]featureA+[no]featureB+...
-static bool DecodeARMFeatures(const Driver &D, StringRef text,
-                              StringRef CPU, llvm::ARM::ArchKind ArchKind,
-                              std::vector<StringRef> &Features) {
+static bool DecodeARMFeatures(const Driver &D, StringRef text, StringRef CPU,
+                              llvm::ARM::ArchKind ArchKind,
+                              std::vector<StringRef> &Features,
+                              unsigned &ArgFPUID) {
   SmallVector<StringRef, 8> Split;
   text.split(Split, StringRef("+"), -1, false);
 
   for (StringRef Feature : Split) {
-    if (!appendArchExtFeatures(CPU, ArchKind, Feature, Features))
+    if (!appendArchExtFeatures(CPU, ArchKind, Feature, Features, ArgFPUID))
       return false;
   }
   return true;
 }
 
 static void DecodeARMFeaturesFromCPU(const Driver &D, StringRef CPU,
                                      std::vector<StringRef> &Features) {
   CPU = CPU.split("+").first;
   if (CPU != "generic") {
     llvm::ARM::ArchKind ArchKind = llvm::ARM::parseCPUArch(CPU);
     uint64_t Extension = llvm::ARM::getDefaultExtensions(CPU, ArchKind);
     llvm::ARM::getExtensionFeatures(Extension, Features);
   }
 }
 
 // Check if -march is valid by checking if it can be canonicalised and parsed.
 // getARMArch is used here instead of just checking the -march value in order
 // to handle -march=native correctly.
 static void checkARMArchName(const Driver &D, const Arg *A, const ArgList &Args,
                              llvm::StringRef ArchName, llvm::StringRef CPUName,
                              std::vector<StringRef> &Features,
-                             const llvm::Triple &Triple) {
+                             const llvm::Triple &Triple, unsigned &ArgFPUID) {
   std::pair<StringRef, StringRef> Split = ArchName.split("+");
 
   std::string MArch = arm::getARMArch(ArchName, Triple);
   llvm::ARM::ArchKind ArchKind = llvm::ARM::parseArch(MArch);
   if (ArchKind == llvm::ARM::ArchKind::INVALID ||
-      (Split.second.size() && !DecodeARMFeatures(
-        D, Split.second, CPUName, ArchKind, Features)))
+      (Split.second.size() && !DecodeARMFeatures(D, Split.second, CPUName,
+                                                 ArchKind, Features, ArgFPUID)))
     D.Diag(clang::diag::err_drv_clang_unsupported) << A->getAsString(Args);
 }
 
 // Check -mcpu=. Needs ArchName to handle -mcpu=generic.
 static void checkARMCPUName(const Driver &D, const Arg *A, const ArgList &Args,
                             llvm::StringRef CPUName, llvm::StringRef ArchName,
                             std::vector<StringRef> &Features,
-                            const llvm::Triple &Triple) {
+                            const llvm::Triple &Triple, unsigned &ArgFPUID) {
   std::pair<StringRef, StringRef> Split = CPUName.split("+");
 
   std::string CPU = arm::getARMTargetCPU(CPUName, ArchName, Triple);
   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)))
+      (Split.second.size() &&
+       !DecodeARMFeatures(D, Split.second, CPU, ArchKind, Features, ArgFPUID)))
     D.Diag(clang::diag::err_drv_clang_unsupported) << 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.getOS() == llvm::Triple::UnknownOS || isARMMProfile(T);

 
 static bool hasIntegerMVE(const std::vector<StringRef> &F) {
   auto MVE = llvm::find(llvm::reverse(F), "+mve");
   auto NoMVE = llvm::find(llvm::reverse(F), "-mve");
   return MVE != F.rend() &&
          (NoMVE == F.rend() || std::distance(MVE, NoMVE) > 0);
 }
 
 void arm::getARMTargetFeatures(const Driver &D, const llvm::Triple &Triple,
                                const ArgList &Args, ArgStringList &CmdArgs,
                                std::vector<StringRef> &Features, bool ForAS) {
   bool KernelOrKext =
       Args.hasArg(options::OPT_mkernel, options::OPT_fapple_kext);
   arm::FloatABI ABI = arm::getARMFloatABI(D, Triple, Args);
   arm::ReadTPMode ThreadPointer = arm::getReadTPMode(D, Args);
   const Arg *WaCPU = nullptr, *WaFPU = nullptr;
   const Arg *WaHDiv = nullptr, *WaArch = nullptr;
 
   // This vector will accumulate features from the architecture
   // extension suffixes on -mcpu and -march (e.g. the 'bar' in
   // -mcpu=foo+bar). We want to apply those after the features derived
   // from the FPU, in case -mfpu generates a negative feature which
   // the +bar is supposed to override.
   std::vector<StringRef> ExtensionFeatures;
 
   if (!ForAS) {
void arm::getARMTargetFeatures(const Driver &D, const llvm::Triple &Triple,
 
   if (ThreadPointer == arm::ReadTPMode::Cp15)
     Features.push_back("+read-tp-hard");
 
   const Arg *ArchArg = Args.getLastArg(options::OPT_march_EQ);
   const Arg *CPUArg = Args.getLastArg(options::OPT_mcpu_EQ);
   StringRef ArchName;
   StringRef CPUName;
+  unsigned ArchArgFPUID = llvm::ARM::FK_INVALID;
+  unsigned CPUArgFPUID = llvm::ARM::FK_INVALID;
 
   // Check -mcpu. ClangAs gives preference to -Wa,-mcpu=.
   if (WaCPU) {
     if (CPUArg)
       D.Diag(clang::diag::warn_drv_unused_argument)
           << CPUArg->getAsString(Args);
     CPUName = StringRef(WaCPU->getValue()).substr(6);
     CPUArg = WaCPU;
   } else if (CPUArg)
     CPUName = CPUArg->getValue();
 
   // Check -march. ClangAs gives preference to -Wa,-march=.
   if (WaArch) {
     if (ArchArg)
       D.Diag(clang::diag::warn_drv_unused_argument)
           << ArchArg->getAsString(Args);
     ArchName = StringRef(WaArch->getValue()).substr(7);
-    checkARMArchName(D, WaArch, Args, ArchName, CPUName,
-                     ExtensionFeatures, Triple);
+    checkARMArchName(D, WaArch, Args, ArchName, CPUName, ExtensionFeatures,
+                     Triple, ArchArgFPUID);
     // FIXME: Set Arch.
     D.Diag(clang::diag::warn_drv_unused_argument) << WaArch->getAsString(Args);
   } else if (ArchArg) {
     ArchName = ArchArg->getValue();
-    checkARMArchName(D, ArchArg, Args, ArchName, CPUName,
-                     ExtensionFeatures, Triple);
+    checkARMArchName(D, ArchArg, Args, ArchName, CPUName, ExtensionFeatures,
+                     Triple, ArchArgFPUID);
   }
 
   // Add CPU features for generic CPUs
   if (CPUName == "native") {
     llvm::StringMap<bool> HostFeatures;
     if (llvm::sys::getHostCPUFeatures(HostFeatures))
       for (auto &F : HostFeatures)
         Features.push_back(
             Args.MakeArgString((F.second ? "+" : "-") + F.first()));
   } else if (!CPUName.empty()) {
     // This sets the default features for the specified CPU. We certainly don't
     // want to override the features that have been explicitly specified on the
     // command line. Therefore, process them directly instead of appending them
     // at the end later.
     DecodeARMFeaturesFromCPU(D, CPUName, Features);
   }
 
   if (CPUArg)
-    checkARMCPUName(D, CPUArg, Args, CPUName, ArchName,
-                    ExtensionFeatures, Triple);
+    checkARMCPUName(D, CPUArg, Args, CPUName, ArchName, ExtensionFeatures,
+                    Triple, CPUArgFPUID);
   // Honor -mfpu=. ClangAs gives preference to -Wa,-mfpu=.
   unsigned FPUID = llvm::ARM::FK_INVALID;
   const Arg *FPUArg = Args.getLastArg(options::OPT_mfpu_EQ);
   if (WaFPU) {
     if (FPUArg)
       D.Diag(clang::diag::warn_drv_unused_argument)
           << FPUArg->getAsString(Args);
     (void)getARMFPUFeatures(D, WaFPU, Args, StringRef(WaFPU->getValue()).substr(6),
fp16_fml_fallthrough:
     // In both of these cases, putting the 'other' feature on the end of the vector will
     // result in the same effect as placing it immediately after the current feature.
     if (ItRNoFullFP16 < ItRFP16FML)
       Features.push_back("-fp16fml");
     else if (ItRNoFullFP16 > ItRFP16FML)
       Features.push_back("+fullfp16");
   }
 
-  // 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.
+  // 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.
   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"});
+  } else if (FPUID == llvm::ARM::FK_NONE ||
+             ArchArgFPUID == llvm::ARM::FK_NONE ||
+             CPUArgFPUID == 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", "-mve", "-mve.fp", "-fpregs"});
-  } else if (FPUID == llvm::ARM::FK_NONE) {
-    // -mfpu=none is *very* similar to -mfloat-abi=soft, only that it should not
-    // disable MVE-I.
-    Features.insert(Features.end(), {"-dotprod", "-fp16fml", "-mve.fp"});
+                    {"-dotprod", "-fp16fml", "-bf16", "-mve.fp"});
     if (!hasIntegerMVE(Features))
       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");

clang/test/CodeGen/arm-bf16-softfloat.c

-// RUN: not %clang -o %t.out -target arm-arm-eabi -march=armv8-a+bf16 -mfloat-abi=soft -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target arm-arm-none-eabi -march=armv8-a+bf16 -mfloat-abi=soft -c %s -o %t 2>&1 | FileCheck %s
+// RUN: not %clang -target arm-arm-none-eabi -march=armv8-a+bf16 -mfpu=none -c %s -o %t 2>&1 | FileCheck %s
+// RUN: not %clang -target arm-arm-none-eabi -march=armv8-a+bf16+nofp -c %s -o %t 2>&1 | FileCheck %s
+// RUN: not %clang -target arm-arm-none-eabi -march=armv8-a+bf16+fp+nofp -c %s -o %t 2>&1 | FileCheck %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv8-a+bf16+fp -c %s -o %t
+// RUN: %clang -target arm-arm-none-eabi -march=armv8-a+bf16+nofp+fp -c %s -o %t
 
 // CHECK: error: __bf16 is not supported on this target
 extern __bf16 var;

clang/test/Driver/arm-nofp-disabled-features.c

+// RUN: %clang -target arm-arm-none-eabi -mfloat-abi=soft %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MFLOAT-ABI-SOFT
+// CHECK-MFLOAT-ABI-SOFT: "-target-feature" "-dotprod"
+// CHECK-MFLOAT-ABI-SOFT: "-target-feature" "-fp16fml"
+// CHECK-MFLOAT-ABI-SOFT: "-target-feature" "-bf16"
+// CHECK-MFLOAT-ABI-SOFT: "-target-feature" "-mve"
+// CHECK-MFLOAT-ABI-SOFT: "-target-feature" "-mve.fp"
+// CHECK-MFLOAT-ABI-SOFT: "-target-feature" "-fpregs"
+
+// RUN: %clang -target arm-arm-none-eabi -mfpu=none %s -### 2>&1 | FileCheck %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv8-a+nofp %s -### 2>&1 | FileCheck %s
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-a35+nofp %s -### 2>&1 | FileCheck %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv8-a+nofp+nomve %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-NOMVE
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-a35+nofp+nomve %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-NOMVE
+// CHECK: "-target-feature" "-dotprod"
+// CHECK: "-target-feature" "-fp16fml"
+// CHECK: "-target-feature" "-bf16"
+// CHECK: "-target-feature" "-mve.fp"
+// CHECK-NOMVE: "-target-feature" "-fpregs"

llvm/include/llvm/Support/ARMTargetParser.h

 
 StringRef getArchName(ArchKind AK);
 unsigned getArchAttr(ArchKind AK);
 StringRef getCPUAttr(ArchKind AK);
 StringRef getSubArch(ArchKind AK);
 StringRef getArchExtName(uint64_t ArchExtKind);
 StringRef getArchExtFeature(StringRef ArchExt);
 bool appendArchExtFeatures(StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
-                           std::vector<StringRef> &Features);
+                           std::vector<StringRef> &Features,
+                           unsigned &ArgFPUKind);
 StringRef getHWDivName(uint64_t HWDivKind);
 
 // Information by Name
 unsigned getDefaultFPU(StringRef CPU, ArchKind AK);
 uint64_t getDefaultExtensions(StringRef CPU, ArchKind AK);
 StringRef getDefaultCPU(StringRef Arch);
 StringRef getCanonicalArchName(StringRef Arch);
 StringRef getFPUSynonym(StringRef FPU);

llvm/lib/Support/ARMTargetParser.cpp

if (CandidateFPU.FPUVer == InputFPU.FPUVer &&
       return CandidateFPU.ID;
     }
   }
 
   // nothing found
   return ARM::FK_INVALID;
 }
 
-bool ARM::appendArchExtFeatures(
-  StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
-  std::vector<StringRef> &Features) {
+bool ARM::appendArchExtFeatures(StringRef CPU, ARM::ArchKind AK,
+                                StringRef ArchExt,
+                                std::vector<StringRef> &Features,
+                                unsigned &ArgFPUID) {
 
   size_t StartingNumFeatures = Features.size();
   const bool Negated = stripNegationPrefix(ArchExt);
   uint64_t ID = parseArchExt(ArchExt);
 
   if (ID == AEK_INVALID)
     return false;
 
if (ArchExt == "fp.dp") {
         return true;
       }
       FPUKind = findDoublePrecisionFPU(getDefaultFPU(CPU, AK));
     } else if (Negated) {
       FPUKind = ARM::FK_NONE;
     } else {
       FPUKind = getDefaultFPU(CPU, AK);
     }
+    ArgFPUID = FPUKind;
     return ARM::getFPUFeatures(FPUKind, Features);
   }
   return StartingNumFeatures != Features.size();
 }
 
 StringRef ARM::getHWDivName(uint64_t HWDivKind) {
   for (const auto D : HWDivNames) {
     if (HWDivKind == D.ID)

llvm/unittests/Support/TargetParserTest.cpp

for (unsigned i = 0; i < array_lengthof(ArchExt); i++) {
     EXPECT_EQ(StringRef(ArchExt[i][3]), ARM::getArchExtFeature(ArchExt[i][1]));
   }
 }
 
 static bool
 testArchExtDependency(const char *ArchExt,
                       const std::initializer_list<const char *> &Expected) {
   std::vector<StringRef> Features;
+  unsigned FPUID;
 
   if (!ARM::appendArchExtFeatures("", ARM::ArchKind::ARMV8_1MMainline, ArchExt,
-                                  Features))
+                                  Features, FPUID))
     return false;
 
   return llvm::all_of(Expected, [&](StringRef Ext) {
     return llvm::is_contained(Features, Ext);
   });
 }
 
 TEST(TargetParserTest, ARMArchExtDependencies) {