[PATCH] D62998: [ARM] Fix bugs introduced by the fp64/d32 rework.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham marked 2 inline comments as done.
simon_tatham added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:449
     //        as well.
-    for (std::string Feature : {"vfp2", "vfp3", "vfp4", "fp-armv8", "fullfp16",
-                                "neon", "crypto", "dotprod", "fp16fml"})
-      if (std::find(std::begin(Features), std::end(Features), "+" + Feature) != std::end(Features))
-        Features.push_back(Args.MakeArgString("-" + Feature));
-
-    // Disable the base feature unconditionally, even if it was not
-    // explicitly in the features list (e.g. if we had +vfp3, which
-    // implies it).
-    Features.push_back("-fpregs");
+    for (std::string Feature : {
+            "vfp2", "vfp2sp", "vfp2d16", "vfp2d16sp",
----------------
SjoerdMeijer wrote:
> This function and this code is truly one of the worst I guess.... I mean, it's not worse than it was before, but you'd really hope that there was a (targetparser) API that gives you a list of FPUs, possibly matching some criteria, rather than yet another list of hard coded strings. It gets worse, this is repeated in `lib/Support/ARMTargetParser.cpp`. End of rant! ;-)
> 
> More on topic: do we need to add +mve.fp here?
It did occur to me that this code could be made a lot simpler if clang had direct access to the data about the dependency relations between subtarget features. Then it could replace all of this with a simpler and more robust set of tests for things like "does the transitive closure of the feature set include `vfp4d16sp`?" instead of having to list every individual feature that //might// appear. But currently that dependency web lives in the ARM target's tablegen, so it isn't guaranteed to be compiled in to the LLVM that clang is linking against.


================
Comment at: llvm/lib/Support/ARMTargetParser.cpp:178
+
+    {"+fpregs", "-fpregs", FPUVersion::VFPV2, FPURestriction::SP_D16},
+    {"+vfp2", "-vfp2", FPUVersion::VFPV2, FPURestriction::None},
----------------
SjoerdMeijer wrote:
> This looks familiar....  I don't think we can easily simplify this? Or get rid of? I guess that's best done in a follow up when we this working again? Perhaps the only insignificant simplification is that we don't need the +<string> and -<string> because they are the same.
As I say in the comment a few lines up, we need both versions of the string because this function is returning a vector of `StringRef` with no mechanism to free anything it allocates, so it has to return pointers to static string literals.

(I could do some preprocessor fiddling to expand a single feature name into both strings, but I tend to assume most people have a less strong stomach for cpp horrors than I do :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62998/new/

https://reviews.llvm.org/D62998