[PATCH] D53121: [Driver] Add defaults for Android ARM FPUs.

[Dan Albert via Phabricator via cfe-commits]

danalbert added inline comments.


================
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:360
 
+  unsigned ArchVersion;
+  if (ArchName.empty())
----------------
peter.smith wrote:
> Do you need to parse the arch version here? I would expect the -march=armv7 to be reflected in getARMSubArchVersionNumber(Triple)
> 
> If reduce this to ArchVersion = getARMSubArchVersionNumber(Triple) and add a test with -target arm-linux-androideabi21 -march=armv7 then everything still passes.
> 
> If I'm right you should be able to simplify this and perhaps roll it into the if (Triple.isAndroid() && ArchVersion >= 7) below. If I'm wrong can we add a test case that fails without the ArchVersion = llvm::ARM::parseArchVersion(ArchName) ?
> 
> It will also be worth adding tests for a generic target with -march 
I had assumed that that wouldn't handle a case like `-target arm-linux-androideabi21 -march=armv7-a` where the `-target` argument specifies ARMv5 but `-march` rasies that to ARMv7, but you're right, the triple is updated to account for `-march` (which makes sense now that I think about it; that would lead to a lot of messy code all over if it didn't). Cleaned this up.

(I think I've also added the test you're asking for, but lmk if I misunderstood what you meant by "generic target")


================
Comment at: test/Driver/arm-mfpu.c:410
+
+// RUN: %clang -target armv7-linux-androideabi23 %s -mfpu=vfp3-d16 -### -c 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ARM-ANDROID-M-FP-D16 %s
----------------
>>! In D53121#1261602, @kristof.beyls wrote:
> Seems fine to me too. I'd maybe just add an additional test case to verify that things still work as expected when users explicitly specify that they want to target a different FPU (e.g. "-mfpu=none").

Is this test (and it's counterpart in `CHECK-ARM-ANDROID-L-FP-NEON`) not sufficient? It shows that `-mfpu` is honored regardless of the default. Is there something special about `-mfpu=none` that this doesn't exercise?


Repository:
  rC Clang

https://reviews.llvm.org/D53121