This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Add defaults for Android ARM FPUs.
ClosedPublic

Authored by danalbert on Oct 10 2018, 5:42 PM.

Details

Summary

Android mandates that devices have at least vfpv3-d16 until
Marshmallow and NEON after that. Still honor the user's decision, but
raise the defaults for Android targets.

Diff Detail

Repository
rC Clang

Event Timeline

danalbert created this revision.Oct 10 2018, 5:42 PM

Related to this but something I was less sure we should do: Android no longer supports ARMv5. Should we make arm-linux-androideabi targets auto pull up to armv7 if there's no -march flag?

srhines added a subscriber: rengolin.

This LGTM, but we should wait to hear from Kristof before submitting.

This LGTM, but we should wait to hear from Kristof before submitting.

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").

It looks like we might be able to simplify a bit, and I think there probably could be some more test cases but no objections from me.

lib/Driver/ToolChains/Arch/ARM.cpp
360

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

danalbert added inline comments.Oct 11 2018, 1:20 PM
lib/Driver/ToolChains/Arch/ARM.cpp
360

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")

test/Driver/arm-mfpu.c
410

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?

danalbert updated this revision to Diff 169291.Oct 11 2018, 1:21 PM

Addressed review comments.

danalbert marked 2 inline comments as done.Oct 11 2018, 2:02 PM
kristof.beyls added inline comments.Oct 12 2018, 1:44 AM
test/Driver/arm-mfpu.c
410

you're right - this test does what I was asking for. Apologies, I should've looked more closely - I hadn't picked up this mfpu isn't the default one...

peter.smith accepted this revision.Oct 12 2018, 1:47 AM

Looks good to me. By generic target I just meant --target=arm-linux-androideabi21 with a -march to specify the revision, which you've got in the test.

This revision is now accepted and ready to land.Oct 12 2018, 1:47 AM
danalbert marked 2 inline comments as done.Oct 12 2018, 9:59 AM
This revision was automatically updated to reflect the committed changes.