[PATCH] D40256: [ARM] disable FPU features when using soft floating point.

[Keith Walker via Phabricator via cfe-commits]

keith.walker.arm added inline comments.


================
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:406
+    const bool HasVFPv4  = (std::find(ItBegin, ItEnd, "+vfpv4") != ItEnd);
+    const bool HasFParmv8  = (std::find(ItBegin, ItEnd, "+fp-armv8") != ItEnd);
+    const bool HasFullFP16  = (std::find(ItBegin, ItEnd, "+fullfp16") != ItEnd);
----------------
efriedma wrote:
> I don't like explicitly enumerating the features like this; it'll mess up if there's ever a new feature which isn't explicitly enumerated here.  Can we just do `Features.push_back("-vfpv2")` and depend on that to implicitly disable all the other vfp features?
I'll check on whether disabling a feature on which other features depend automatically disables the other features is something that can be relied upon (It would seem sensible that one could .... but need to check).

That would certainly mean the code could be simplified, although I would also need to check the impact on the what is checked in the testing (only test for the base feature being disabled because the dependent features are automatically disabled).


================
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:419
+        Features.push_back("-fullfp16");
+
+    const bool HasNeon  = (std::find(ItBegin, ItEnd, "+neon") != ItEnd);
----------------
compnerd wrote:
> It would be nice to not have these explicitly listed.  But at the very least, I think that having a list and looping through it would be better:
> 
>     for (const auto Feature : {"vfpv2", "vfpv3", "vfpv4", "fp-armv8", "fullfp16"})
>       if (std::find(std::begin(Features), std::end(Features), "+" + Feature) == std::end(Features))
>         continue;
>       else
>         Features.push_back("-" + Feature);
This certainly looks a better way to do it if we do need to provide a list of features rather than relying on disabling a base feature on which the other features depend. 


https://reviews.llvm.org/D40256