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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
