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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:103
                              std::vector<StringRef> &Features,
+                             std::vector<StringRef> &FeaturesAfter,
                              const llvm::Triple &Triple) {
----------------
Nitpick: can you comment in code somewhere what the purpose is of `FeaturesAfter`? Can it be renamed to something more descriptive? I don't know what it could be to be honest...


================
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",
----------------
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?


================
Comment at: llvm/lib/Support/ARMTargetParser.cpp:178
+
+    {"+fpregs", "-fpregs", FPUVersion::VFPV2, FPURestriction::SP_D16},
+    {"+vfp2", "-vfp2", FPUVersion::VFPV2, FPURestriction::None},
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62998