[Targets] Move soft-float-abi filtering to `initFeatureMap`

[george.burgess.iv]

Summary

I'm not convinced this is an excellent approach, in part because I'm unclear on where all we expect to funnel the results of TargetInfo::initFeatureMap. Thought I'd throw it out for review anyway, and see what people with actual context think. :)

The underlying problem I'm trying to solve is that, given the following code with a suitable ARM target,

___attribute__((target("crc"))) void foo() {}

clang ends up codegening a function with the '+soft-float-abi' target feature, which we go out of our way to remove in the frontend for the default target features, since the backend apparently tries to figure out whether the soft float ABI should be used on its own. This is more or less harmless on its own, but it causes the backend to emit a diagnostic directly to errs(). Rather than try to find a way to silence that diag, it seems better to not have to emit it in the first place.

An alternate fix would be to somehow try to filter this in cfe/lib/CodeGen, but there appear to be many callers of initFeatureMap; I get the vague feeling that we shouldn't be letting +soft-float-abi slip through any of them. Again, could be wrong about that due to my lack of familiarity with initFeatureMap's uses.

If we agree that this is a good way forward, there also appears to be +neonfp/-neonfp additions happening in handleTargetFeatures that should prooooobably be happening in initFeatureMap instead? If that's the case, I'm happy to do that as a follow-up, and make it so that handleTargetFeatures no longer mutates its input (which comments indicate would be desirable, along with a more general move of all of this initialization stuff to the construction of our various TargetInfo subclasses).

[efriedma]

If we agree that this is a good way forward, there also appears to be +neonfp/-neonfp additions happening in handleTargetFeatures that should prooooobably be happening in initFeatureMap instead?

neonfp isn't passed as a feature in the first place; there's a separate API setFPMath which is used for that. We translate it into a target feature for the sake of the backend. So I'm not sure what you're proposing.

---

What happens if someone specifies attribute((target("soft-float-abi"))) or something like that? (There's an API isValidFeatureName to validate feature names, but we currently don't implement it.)

[george.burgess.iv]

Address comments -- thanks!

[george.burgess.iv]

neonfp isn't passed as a feature in the first place; there's a separate API setFPMath which is used for that. We translate it into a target feature for the sake of the backend. So I'm not sure what you're proposing.

The idea would be for initFeatureMap to handle adding neonfp instead, so we can make the vector& we're passing into handleTargetFeatures a const vector&. An old comment claims that this would be desirable from a refactoring standpoint

What happens if someone specifies attribute((target("soft-float-abi"))) or something like that? (There's an API isValidFeatureName to validate feature names, but we currently don't implement it.)

Good catch. It doesn't appear that that works on its own for the target attribute, since we only filter user attrs, but it's probably something that's good to diagnose.

[george.burgess.iv]

ping :)

[efriedma]

LGTM

clang/lib/Basic/Targets/ARM.h

public:
   bool setABI(const std::string &Name) override;
 
   // FIXME: This should be based on Arch attributes, not CPU names.
   bool
   initFeatureMap(llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags,
                  StringRef CPU,
                  const std::vector<std::string> &FeaturesVec) const override;
 
+  bool isValidFeatureName(StringRef Feature) const override {
+    // We pass soft-float-abi in as a -target-feature, but the backend figures
+    // this out through other means.
+    return Feature != "soft-float-abi";
+  }
+
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override;
 
   bool hasFeature(StringRef Feature) const override;
 
   bool isValidCPUName(StringRef Name) const override;
   void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
 

clang/lib/Basic/Targets/ARM.cpp

ARMTargetInfo::ARMTargetInfo(const llvm::Triple &Triple,
   // zero length bitfield.
   UseZeroLengthBitfieldAlignment = true;
 
   if (Triple.getOS() == llvm::Triple::Linux ||
       Triple.getOS() == llvm::Triple::UnknownOS)
     this->MCountName = Opts.EABIVersion == llvm::EABI::GNU
                            ? "\01__gnu_mcount_nc"
                            : "\01mcount";
+
+  SoftFloatABI = llvm::is_contained(Opts.FeaturesAsWritten, "+soft-float-abi");
 }
 
 StringRef ARMTargetInfo::getABI() const { return ABI; }
 
 bool ARMTargetInfo::setABI(const std::string &Name) {
   ABI = Name;
 
   // The defaults (above) are for AAPCS, check if we need to change them.
bool ARMTargetInfo::initFeatureMap(
   // ARM and Thumb code generation.
   if (isThumb())
     Features["thumb-mode"] = true;
   else
     Features["thumb-mode"] = false;
 
   // Convert user-provided arm and thumb GNU target attributes to
   // [-|+]thumb-mode target features respectively.
-  std::vector<std::string> UpdatedFeaturesVec(FeaturesVec);
-  for (auto &Feature : UpdatedFeaturesVec) {
-    if (Feature.compare("+arm") == 0)
-      Feature = "-thumb-mode";
-    else if (Feature.compare("+thumb") == 0)
-      Feature = "+thumb-mode";
+  std::vector<std::string> UpdatedFeaturesVec;
+  for (const auto &Feature : FeaturesVec) {
+    // Skip soft-float-abi; it's something we only use to initialize a bit of
+    // class state, and is otherwise unrecognized.
+    if (Feature == "+soft-float-abi")
+      continue;
+
+    StringRef FixedFeature;
+    if (Feature == "+arm")
+      FixedFeature = "-thumb-mode";
+    else if (Feature == "+thumb")
+      FixedFeature = "+thumb-mode";
+    else
+      FixedFeature = Feature;
+    UpdatedFeaturesVec.push_back(FixedFeature.str());
   }
 
   return TargetInfo::initFeatureMap(Features, Diags, CPU, UpdatedFeaturesVec);
 }
 
 
 bool ARMTargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
                                          DiagnosticsEngine &Diags) {
   FPU = 0;
   CRC = 0;
   Crypto = 0;
   DSP = 0;
   Unaligned = 1;
-  SoftFloat = SoftFloatABI = false;
+  SoftFloat = false;
+  // Note that SoftFloatABI is initialized in our constructor.
   HWDiv = 0;
   DotProd = 0;
   HasFloat16 = true;
 
   // This does not diagnose illegal cases like having both
   // "+vfpv2" and "+vfpv3" or having "+neon" and "+fp-only-sp".
   uint32_t HW_FP_remove = 0;
   for (const auto &Feature : Features) {
     if (Feature == "+soft-float") {
       SoftFloat = true;
-    } else if (Feature == "+soft-float-abi") {
-      SoftFloatABI = true;
     } else if (Feature == "+vfp2") {
       FPU |= VFP2FPU;
       HW_FP |= HW_FP_SP | HW_FP_DP;
     } else if (Feature == "+vfp3") {
       FPU |= VFP3FPU;
       HW_FP |= HW_FP_SP | HW_FP_DP;
     } else if (Feature == "+vfp4") {
       FPU |= VFP4FPU;
if (!(FPU & NeonFPU) && FPMath == FP_Neon) {
     return false;
   }
 
   if (FPMath == FP_Neon)
     Features.push_back("+neonfp");
   else if (FPMath == FP_VFP)
     Features.push_back("-neonfp");
 
-  // Remove front-end specific options which the backend handles differently.
-  auto Feature = llvm::find(Features, "+soft-float-abi");
-  if (Feature != Features.end())
-    Features.erase(Feature);
-
   return true;
 }
 
 bool ARMTargetInfo::hasFeature(StringRef Feature) const {
   return llvm::StringSwitch<bool>(Feature)
       .Case("arm", true)
       .Case("aarch32", true)
       .Case("softfloat", SoftFloat)

clang/test/CodeGen/arm-soft-float-abi-filtering.c

+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -emit-llvm -o - -triple arm-none-linux-gnueabi -target-feature "+soft-float-abi" %s | FileCheck %s
+// RUN: %clang_cc1 -verify -triple arm-none-linux-gnueabi -target-feature "+soft-float-abi" %s
+
+// CHECK-NOT: +soft-float-abi
+
+void foo() {}
+__attribute__((target("crc"))) void bar() {}
+__attribute__((target("soft-float-abi"))) void baz() {} // expected-warning{{unsupported 'soft-float-abi' in the 'target' attribute string}}