[ARM] Allow "-march=foo+fp" to vary with foo.

[SjoerdMeijer]

Summary

Now, when clang processes an argument of the form "-march=foo+x+y+z",
then instead of calling getArchExtFeature() for each of the extension
names "x", "y", "z" and appending the returned string to its list of
low-level subtarget features, it will call appendArchExtFeatures()
which does the appending itself.

The difference is that appendArchExtFeatures can add _more_ than one
low-level feature name to the output feature list if it has to, and
also, it gets told some information about what base architecture and
CPU the extension is going to go with, which means that "+fp" can now
mean something different for different CPUs. Namely, "+fp" now selects
whatever the _default_ FPU is for the selected CPU and/or
architecture, as defined in the ARM_ARCH or ARM_CPU_NAME macros in
ARMTargetParser.def.

On the clang side, I adjust DecodeARMFeatures to call the new
appendArchExtFeatures function in place of getArchExtFeature. This
means DecodeARMFeatures needs to be passed a CPU name and an ArchKind,
which meant changing its call sites to make those available, and also
sawing getLLVMArchSuffixForARM in half so that you can get an ArchKind
enum value out of it instead of a string.

Also, I add support here for the extension name "+fp.dp", which will
automatically look through the FPU list for something that looks just
like the default FPU except for also supporting double precision.

[t.p.northover]

This needs some tests. I'm also not quite sure when you'd use bare "+fp", if it's the default anyway.

llvm/lib/Support/ARMTargetParser.cpp

487–490

I think isNegated is probably more in line with existing naming.

515–518

Doesn't this mean +nofp.dp disables all floats? That seems surprising behaviour to me, but I can't find any existing tests covering it.

527–528

Doesn't this silently disable the FPU entirely if we decide "fp.dp" is useless? That seems unlikely to be what a user wants, especially without a diagnostic.

Could you also expand on the comment a bit more. I had to look up exactly what FPURestrictions existed to get this, and I'm not even 100% sure I'm right now.

[simon_tatham]

The aim of this change is that it will apply to the v8.1-M (mainline) architecture introduced in D60698, in which +fp won't be the default: -march=armv8.1m.main by itself gives you the base 8.1-M architecture without any FP, -march=armv8.1m.main+fp gives you the optional single-precision FP extension on top of that, and +fp.dp gives you double precision as well.

llvm/lib/Support/ARMTargetParser.cpp

487–490

Hmmm. I thought that would be a confusing name because it hides the fact that the function strips off the no prefix. (The use of 'was' was intended to hint that by the time the function returns, it's not true any more!)

Perhaps stripNegationPrefix (returning bool to indicate success)?

515–518

Hmmm, that's a good point. What would a user expect in that situation? If double-precision FP was the default for that architecture and a single-precision version existed, then perhaps nofp.dp should fall back to that, but what if it's double or nothing?

527–528

I don't think it silently disables it, does it? Returning false from this function is a failure indication that ends up back in checkARMArchName in clang/lib/Driver/ToolChains/Arch/ARM.cpp, which will generate a diagnostic. For example, if I try -march=armv6m+fp.dp then I see

error: the clang compiler does not support '-march=armv6m+fp.dp'

[t.p.northover]

llvm/lib/Support/ARMTargetParser.cpp

487–490

Ah yes, I see. I think your alternative is probably better.

515–518

I think I'd go for a diagnostic in that case. There's already a way to strip out the FPU then (+nofp).

527–528

Ah, I missed the only way return true could happen and assumed the return value was vestigial. Sorry.

[SjoerdMeijer]

This addresses @t.p.northover comment.

[ostannard]

This still needs tests adding.

[SjoerdMeijer]

Ah yes, the school boy error! ;-) Actually, there was a test, but in a different patch; I will move it to here.

[SjoerdMeijer]

This time with tests.

[ostannard]

llvm/lib/Support/ARMTargetParser.cpp

551

Could you also add tests for the error cases here? I think these are:

• +fp.dp, but the FPU is already double-precision
• +fp.dp, but no double-precision FPU exists (are there any FPUs which cause this?)
• +[no]fp or +[no]fp.dp for a CPU/arch which doesn't have any FPUs.

I also don't see any tests for the negated forms of either feature.

[SjoerdMeijer]

Hi Oliver, thanks for your comments!

This was the easy one, they have been added:

I also don't see any tests for the negated forms of either feature.

The trouble begun with this:

+fp.dp, but the FPU is already double-precision
+fp.dp, but no double-precision FPU exists (are there any FPUs which cause this?)
+[no]fp or +[no]fp.dp for a CPU/arch which doesn't have any FPUs.

Because I found that basically none of this worked. The main reason was that we were always passing generic. To address that we at least have a chance of seeing a sensible CPU name, I have swapped the order of parsing -march and -mcpu. I.e., we parse -mcpu first, and pass that to checkARMArchName, which will eventually call appendArchExtFeatures. I think that makes more sense when we use the CPUname to query getDefaultFPU.

Then about the more fancy diagnostics (e.g. "fp.dp, but the FPU is already double-precision"): I've removed any attempt to throw clever diagnostics. I don't think, in general, that we provide this kind of service level. I.e., we need to do a lot more work here to avoid a meaningless, confusing, and thus useless "--march=... not supported" error message when we provide +fp.dp on the -march when e.g. the CPU already enabled this.

[ostannard]

Fair enough, I don't think we currently try to diagnose any other invalid combinations of features. LGTM.

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

 //===--- ARM.h - ARM-specific (not AArch64) Tool Helpers --------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_ARCH_ARM_H
 #define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_ARCH_ARM_H
 
 #include "clang/Driver/ToolChain.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Option/Option.h"
+#include "llvm/Support/TargetParser.h"
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace driver {
 namespace tools {
 namespace arm {
 
 std::string getARMTargetCPU(StringRef CPU, llvm::StringRef Arch,
                             const llvm::Triple &Triple);
 const std::string getARMArch(llvm::StringRef Arch, const llvm::Triple &Triple);
 StringRef getARMCPUForMArch(llvm::StringRef Arch, const llvm::Triple &Triple);
+llvm::ARM::ArchKind getLLVMArchKindForARM(StringRef CPU, StringRef Arch,
+                                          const llvm::Triple &Triple);
 StringRef getLLVMArchSuffixForARM(llvm::StringRef CPU, llvm::StringRef Arch,
                                   const llvm::Triple &Triple);
 
 void appendBE8LinkFlag(const llvm::opt::ArgList &Args,
                        llvm::opt::ArgStringList &CmdArgs,
                        const llvm::Triple &Triple);
 enum class ReadTPMode {
   Invalid,

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

static void 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);
 }
 
 // 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) {
   SmallVector<StringRef, 8> Split;
   text.split(Split, StringRef("+"), -1, false);
 
   for (StringRef Feature : Split) {
-    StringRef FeatureName = llvm::ARM::getArchExtFeature(Feature);
-    if (!FeatureName.empty())
-      Features.push_back(FeatureName);
-    else
+    if (!appendArchExtFeatures(CPU, ArchKind, Feature, Features))
       return false;
   }
   return true;
 }
 
 static void DecodeARMFeaturesFromCPU(const Driver &D, StringRef CPU,
                                      std::vector<StringRef> &Features) {
   CPU = CPU.split("+").first;

 // to handle -march=native correctly.
 static void checkARMArchName(const Driver &D, const Arg *A, const ArgList &Args,
                              llvm::StringRef ArchName,
                              std::vector<StringRef> &Features,
                              const llvm::Triple &Triple) {
   std::pair<StringRef, StringRef> Split = ArchName.split("+");
 
   std::string MArch = arm::getARMArch(ArchName, Triple);
-  if (llvm::ARM::parseArch(MArch) == llvm::ARM::ArchKind::INVALID ||
-      (Split.second.size() && !DecodeARMFeatures(D, Split.second, Features)))
+  llvm::ARM::ArchKind ArchKind = llvm::ARM::parseArch(MArch);
+  if (ArchKind == llvm::ARM::ArchKind::INVALID ||
+      (Split.second.size() && !DecodeARMFeatures(
+        D, Split.second, "generic", ArchKind, Features)))
     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) {
   std::pair<StringRef, StringRef> Split = CPUName.split("+");
 
   std::string CPU = arm::getARMTargetCPU(CPUName, ArchName, Triple);
-  if (arm::getLLVMArchSuffixForARM(CPU, ArchName, Triple).empty() ||
-      (Split.second.size() && !DecodeARMFeatures(D, Split.second, Features)))
+  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)))
     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);
if (MCPU == "native")
       return llvm::sys::getHostCPUName();
     else
       return MCPU;
   }
 
   return getARMCPUForMArch(Arch, Triple);
 }
 
-/// getLLVMArchSuffixForARM - Get the LLVM arch name to use for a particular
-/// CPU  (or Arch, if CPU is generic).
-// FIXME: This is redundant with -mcpu, why does LLVM use this.
-StringRef arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch,
+/// getLLVMArchSuffixForARM - Get the LLVM ArchKind value to use for a
+/// particular CPU (or Arch, if CPU is generic). This is needed to
+/// pass to functions like llvm::ARM::getDefaultFPU which need an
+/// ArchKind as well as a CPU name.
+llvm::ARM::ArchKind arm::getLLVMArchKindForARM(StringRef CPU, StringRef Arch,
                                                const llvm::Triple &Triple) {
   llvm::ARM::ArchKind ArchKind;
   if (CPU == "generic") {
     std::string ARMArch = tools::arm::getARMArch(Arch, Triple);
     ArchKind = llvm::ARM::parseArch(ARMArch);
     if (ArchKind == llvm::ARM::ArchKind::INVALID)
       // In case of generic Arch, i.e. "arm",
       // extract arch from default cpu of the Triple
       ArchKind = llvm::ARM::parseCPUArch(Triple.getARMCPUForArch(ARMArch));
   } else {
     // FIXME: horrible hack to get around the fact that Cortex-A7 is only an
     // armv7k triple if it's actually been specified via "-arch armv7k".
     ArchKind = (Arch == "armv7k" || Arch == "thumbv7k")
                           ? llvm::ARM::ArchKind::ARMV7K
                           : llvm::ARM::parseCPUArch(CPU);
   }
+  return ArchKind;
+}
+
+/// getLLVMArchSuffixForARM - Get the LLVM arch name to use for a particular
+/// CPU  (or Arch, if CPU is generic).
+// FIXME: This is redundant with -mcpu, why does LLVM use this.
+StringRef arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch,
+                                       const llvm::Triple &Triple) {
+  llvm::ARM::ArchKind ArchKind = getLLVMArchKindForARM(CPU, Arch, Triple);
   if (ArchKind == llvm::ARM::ArchKind::INVALID)
     return "";
   return llvm::ARM::getSubArch(ArchKind);
 }
 
 void arm::appendBE8LinkFlag(const ArgList &Args, ArgStringList &CmdArgs,
                             const llvm::Triple &Triple) {
   if (Args.hasArg(options::OPT_r))
     return;
 
   // ARMv7 (and later) and ARMv6-M do not support BE-32, so instruct the linker
   // to generate BE-8 executables.
   if (arm::getARMSubArchVersionNumber(Triple) >= 7 || arm::isARMMProfile(Triple))
     CmdArgs.push_back("--be8");
 }

llvm/include/llvm/Support/ARMTargetParser.h

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

llvm/lib/Support/ARMTargetParser.cpp

 StringRef ARM::getArchExtName(unsigned ArchExtKind) {
   for (const auto AE : ARCHExtNames) {
     if (ArchExtKind == AE.ID)
       return AE.getName();
   }
   return StringRef();
 }
 
-StringRef ARM::getArchExtFeature(StringRef ArchExt) {
-  if (ArchExt.startswith("no")) {
-    StringRef ArchExtBase(ArchExt.substr(2));
-    for (const auto AE : ARCHExtNames) {
-      if (AE.NegFeature && ArchExtBase == AE.getName())
-        return StringRef(AE.NegFeature);
-    }
-  }
+static bool stripNegationPrefix(StringRef &Name) {
+  if (Name.startswith("no")) {
+    Name = Name.substr(2);
+    return true;
+  return false;
+
+StringRef ARM::getArchExtFeature(StringRef ArchExt) {
+  bool Negated = stripNegationPrefix(ArchExt);
   for (const auto AE : ARCHExtNames) {
     if (AE.Feature && ArchExt == AE.getName())
-      return StringRef(AE.Feature);
+      return StringRef(Negated ? AE.NegFeature : AE.Feature);
   }
 
   return StringRef();
 }
 
+static unsigned findDoublePrecisionFPU(unsigned InputFPUKind) {
+  const ARM::FPUName &InputFPU = ARM::FPUNames[InputFPUKind];
+
+  // If the input FPU already supports double-precision, then there
+  // isn't any different FPU we can return here.
+  //
+  // The current available FPURestriction values are None (no
+  // restriction), D16 (only 16 d-regs) and SP_D16 (16 d-regs
+  // and single precision only); there's no value representing
+  // SP restriction without D16. So this test just means 'is it
+  // SP only?'.
+  if (InputFPU.Restriction != ARM::FPURestriction::SP_D16)
+    return ARM::FK_INVALID;
+
+  // Otherwise, look for an FPU entry with all the same fields, except
+  // that SP_D16 has been replaced with just D16, representing adding
+  // double precision and not changing anything else.
+  for (const ARM::FPUName &CandidateFPU : ARM::FPUNames) {
+    if (CandidateFPU.FPUVer == InputFPU.FPUVer &&
+        CandidateFPU.NeonSupport == InputFPU.NeonSupport &&
+        CandidateFPU.Restriction == ARM::FPURestriction::D16) {
+      return CandidateFPU.ID;
+    }
+  }
+
+  // nothing found
+  return ARM::FK_INVALID;
+}
+
+bool ARM::appendArchExtFeatures(
+  StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+  std::vector<StringRef> &Features) {
+  StringRef StandardFeature = getArchExtFeature(ArchExt);
+  if (!StandardFeature.empty()) {
+    Features.push_back(StandardFeature);
+    return true;
+  }
+
+  bool Negated = stripNegationPrefix(ArchExt);
+
+  if (ArchExt == "fp" || ArchExt == "fp.dp") {
+    unsigned FPUKind;
+
+    if (ArchExt == "fp.dp") {
+      unsigned DoubleFPUKind = findDoublePrecisionFPU(getDefaultFPU(CPU, AK));
+
+      // If the default FPU already supports double-precision, or if
+      // there is no double-prec FPU that extends it, then "fp.dp"
+      // doesn't have a separate meaning, and we treat it as an
+      // invalid extension name.
+      if (DoubleFPUKind == FK_INVALID)
+        return false;
+
+      // If there _is_ a separate double-precision FPU, then "nofp.dp"
+      // should disable just the double-precision extension, leaving
+      // the base FPU still enabled if it previously was.
+      if (Negated) {
+        Features.push_back("-fp64");
+        return true;
+      }
+
+      // Otherwise, select the double-precision FPU.
+      FPUKind = DoubleFPUKind;
+    } else if (Negated) {
+      FPUKind = ARM::FK_NONE;
+    } else {
+      FPUKind = getDefaultFPU(CPU, AK);
+      if (FPUKind == ARM::FK_NONE)
+        return false;
+    }
+    return ARM::getFPUFeatures(FPUKind, Features);
+  }
+
+  return false;
+}
+
 StringRef ARM::getHWDivName(unsigned HWDivKind) {
   for (const auto D : HWDivNames) {
     if (HWDivKind == D.ID)
       return D.getName();
   }
   return StringRef();
 }