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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: michaelplatings, efriedma.
Herald added subscribers: kristof.beyls, javed.absar.
Herald added a project: clang.

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).


Repository:
  rC Clang

https://reviews.llvm.org/D61750

Files:
  lib/Basic/Targets/ARM.cpp
  test/CodeGen/arm-soft-float-abi-filtering.c


Index: test/CodeGen/arm-soft-float-abi-filtering.c
===================================================================
--- /dev/null
+++ test/CodeGen/arm-soft-float-abi-filtering.c
@@ -0,0 +1,7 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -emit-llvm -o - -triple arm-none-linux-gnueabi -target-feature "+soft-float-abi" %s | FileCheck %s
+
+// CHECK-NOT: +soft-float-abi
+
+void foo() {}
+__attribute__((target("crc"))) void bar() {}
Index: lib/Basic/Targets/ARM.cpp
===================================================================
--- lib/Basic/Targets/ARM.cpp
+++ lib/Basic/Targets/ARM.cpp
@@ -313,6 +313,8 @@
     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; }
@@ -375,12 +377,21 @@
 
   // 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);
@@ -394,7 +405,8 @@
   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;
@@ -405,8 +417,6 @@
   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;
@@ -475,11 +485,6 @@
   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;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61750.198740.patch
Type: text/x-patch
Size: 2810 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190509/8d81a87e/attachment.bin>