[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

288

That's kinda mouthful name.

292–297

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

292–297

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

[DavidSpickett]

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

296

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

288

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

292–297

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

292–297

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.

296

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

[chill]

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

292–297

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

493

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

493

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

493

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

493

Thanks, lgtm.

[chill]

LGTM, thanks!

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

487

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 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);
 }
 
+static void appendNoFPUnsupportedFeatures(const arm::FloatABI ABI,
+                                          const unsigned FPUID,
+                                          const StringRef &ArchName,
+                                          std::vector<StringRef> &Features) {
+  auto checkFPDisabledInArchName = [](const StringRef &ArchName) {
+    SmallVector<StringRef, 8> Split;
+    ArchName.split(Split, '+', -1, false);
+    return llvm::any_of(
+        Split, [](const StringRef &Extension) { return Extension == "nofp"; });
+  };
+
+  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", "-mve", "-mve.fp", "-fpregs"});
+  } else if (FPUID == llvm::ARM::FK_NONE ||
+             checkFPDisabledInArchName(ArchName)) {
+    // -mfpu=none or -march=armvX+nofp is *very* similar to -mfloat-abi=soft,
+    // only that it should not disable MVE-I.
+    Features.insert(Features.end(), {"-dotprod", "-fp16fml", "-mve.fp"});
+    if (!hasIntegerMVE(Features)) {
+      Features.emplace_back("-fpregs");
+    }
+  }
+}
+
 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;
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.
-  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", "-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"});
-    if (!hasIntegerMVE(Features))
-      Features.emplace_back("-fpregs");
-  }
+  // Setting -msoft-float/-mfloat-abi=soft, -mfpu=none, or adding +nofp to
+  // -march 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.
+  appendNoFPUnsupportedFeatures(ABI, FPUID, ArchName, Features);
 
   // 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");
   }
 
   // For Arch >= ARMv8.0 && A profile:  crypto = sha2 + aes
   // FIXME: this needs reimplementation after the TargetParser rewrite