Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support an armv7-android-linuxeabi target #33278

Closed
froydnj opened this issue Apr 29, 2016 · 22 comments
Closed

support an armv7-android-linuxeabi target #33278

froydnj opened this issue Apr 29, 2016 · 22 comments

Comments

@froydnj
Copy link
Contributor

froydnj commented Apr 29, 2016

Firefox for Android compiles its code with -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp. In rustc terms, I think this would be -C +vfp2,+v7-a,+thumb2. AFAICT, this is somewhat different than the arm-linux-androideabi target, which appears to not assume the existence of an FPU, and also doesn't compile as Thumb where possible. When using Rust on Android, we'd like our Rust libraries to be just as well optimized as our C++ libraries, which means either a) having an official, tier-2 supported target; or b) compiling it ourselves.

One could multiply ARM targets to no end, but this seems like a pretty reasonable target for many Android devices out there. Is it possible to have this target added and supported?

cc @alexcrichton

@nagisa
Copy link
Member

nagisa commented Apr 29, 2016

Firefox … compiles … -mfloat-abi=softfp

arm-linux-androideabi … appears to not assume the existence of an FPU

But the -mfloat-abi=softfp means not using that FPU at all?


Our current nightly has following features enabled for the arm-linux-androideabi triplet: +v7,+vfp3,+d16, which means that we already target armv7 and vfp3 is one version above the vfp2. The only other thing is thumb, which I think could be just added to the current target, if there’s proof android requires it.

@alexcrichton
Copy link
Member

I believe the current ABIs supported by Android itself are listed here and although we have not thoroughly documented what our Android target is going for, it's defacto been going towards armeabi-v7a.

So in that spirit we should just update our target spec to include Thumb (as mentioned by @nagisa), but I'm also curious about the float-abi=softfp that Gecko uses? If we enable hard float (which looks like is part of the ABI according to android), would that cause problems?

@froydnj
Copy link
Contributor Author

froydnj commented Apr 29, 2016

So there are two different choices for how to pass floating-point arguments on ARM:

  • The software floating-point ABI, which dictates that float (f32) arguments are passed integer registers and double (f64) arguments are passed paired integer registers. This is -mfloat-abi=soft, and is typically used for chips that don't have an FPU.
  • The hardware floating-point ABI, which dictates that floating-point arguments are passed in the floating-point registers. This is mfloat-abi=hard (or maybe hardfp), and can be used for chips that do have an FPU.

What does -mfloat-abi=softfp mean, then? It means to pass floating-point arguments in the integer registers, but you can use hardware floating-point instructions under the hood. It also means that you can interoperate with code compiled with -mfloat-abi=soft. As far as I know, we don't want to enable the hardware floating-point ABI, but we do want to compile with hardware floating-point instructions...at least Firefox does. This is also what the armeabi-v7a on the Android ABI page linked by @alexcrichton does:

The armeabi-v7a ABI uses the -mfloat-abi=softfp switch to enforce the rule that the compiler must pass all double values in core register pairs during function calls, instead of dedicated floating-point ones. The system can perform all internal computations using the FP registers. Doing so speeds up the computations greatly.

Thumb is very desirable...the smaller the Rust runtime libraries were, the better from the perspective of our mobile team.

I think we'd want to support vfp2 instead of vfp3,d16; it's the same number of core registers, IIUC, but without using some instructions that some of our devices might not support? I'd have to investigate further on that front.

@Nercury
Copy link
Contributor

Nercury commented Apr 30, 2016

I propose to define android target instruction sets and names based on official ABI list.

although we have not thoroughly documented what our Android target is going for, it's defacto been going towards armeabi-v7a.

I think our android target arm-linux-androideabi has evolved so much that the name does not describe it anymore. As far as users are concerned, it can only work with armeabi-v7a (and up) ABI.

If we look at the official list of ABIs, it is clear that we will want soon to support all the flavors, with instruction sets that are defined there. The wishes what we would want android targets to be are not relevant or would at least be very inconvenient to use.

I propose this path forward:

  • Remove HF and other features from arm-linux-androideabi to match minimal android arm ABI target named armeabi.
  • At the same time, create a new target armv7-linux-androideabi to match android armeabi-v7a. For starters, we can copy existing arm-linux-androideabi there.
  • Follow the same approach with other android targets, such as arm64-v8a, x86, x86_64, mips, mips64.

There is also a precedent here: Rust iOS targets (which have to work on similar archs) are already prepared this way and they are easy to use: armv7-apple-ios, armv7s-apple-ios, i386-apple-ios, aarch64-apple-ios, x86_64-apple-ios.

Also, if we follow this approach, the addition of "hf" to the name is not necessary, because the hf requirement comes implicitly from supporting a particular ABI.

@Nercury
Copy link
Contributor

Nercury commented Apr 30, 2016

@alexcrichton

If we enable hard float (which looks like is part of the ABI according to android), would that cause problems?

The usual approach in android-land when working with native code is to compile native code for multiple ABIs and then combine them into single "fat" apk that can work on different devices. Is is also possible to make "split" apks to reduce apk size. But the main point I want to make here that it would be easiest to do that if there was 1:1 mapping between android ABI and Rust target.

That way, we could be sure we support all range of options with optimal configurations for each.

@froydnj
Copy link
Contributor Author

froydnj commented May 2, 2016

Aligning Rust targets with Android ABI names makes sense to me.

@alexcrichton
Copy link
Member

@Nercury yeah I'm inclined to agree, it definitely seems simplest if everything maps 1:1 with the official ABI list.

cc @larsbergstrom, this may involve changing the definition of arm-linux-androideabi to be much slower than it is today (e.g. no v7/vfp/etc goodies), but adding a new armv7-linux-androideabi target which is the same as today's arm-linux-androideabi. Do you know if that'd cause problems for Servo? To minimize disruption we could have a plan like:

  • Add armv7-linux-androideabi
  • Add automation for those nightlies
  • Once nightlies are confirmed, downgrade arm-linux-androideabi

That way Servo should always have a nightly with "fast Android" ?

@larsbergstrom
Copy link
Contributor

@alexcrichton This should work fine for Servo! We already pass a special target triple to Rust different from the native dependencies on a few other ARM platforms, so we have support for mix-and-match in our driver (mach).

@mbrubeck
Copy link
Contributor

mbrubeck commented May 3, 2016

I think we'd want to support vfp2 instead of vfp3,d16; it's the same number of core registers, IIUC, but without using some instructions that some of our devices might not support? I'd have to investigate further on that front.

The armeabi-v7a ABI requires VFPv3-D16, so this should be safe to use in the armv7 target.

create a new target armv7-linux-androideabi to match android armeabi-v7a

This has one downside that may be unavoidable: The Android NDK toolchain uses the target triple arm-linux-androideabi, not armv7-linux-androideabi. This means that armv7-linux-androideabi will not work for existing crates that assume the rustc target matches the Android NDK target, for example the widely-used gcc crate. All such crates will need to be updated with correct logic to map Rust targets to Android NDK targets.

Lastly, Android itself can be built with a "target variant" armv7-a-neon which is ARMv7 + VFPv3-D32 + NEON. According to JBQ this makes a measureable difference on devices that support it, so it might be worth having Rust stdlib builds available for this target. (Servo would use them, for example.)

@brson
Copy link
Contributor

brson commented May 3, 2016

It concerns me that I don't see google defining target triples for all their ABI's - as @mbrubeck says they seem to just always use arm-linux-androideabi and disambiguate the ABI's some other way. So we're out on a limb defining our own target triples to map to their ABIs.

@Nercury
Copy link
Contributor

Nercury commented May 4, 2016

@brson I guess their idea is that the requested abi carries enough options for arm-linux-androideabi to do the job, however those options also include path to the precompiled libraries to use. If we were to follow this, we would need some option for rust to change what precompiled std libraries to use and what "target variant" to produce.

Of course, the "target variant" must match variant used for std libs.

I am almost done with armv7a-linux-androideabi target, but still waiting for better ideas (if any).

@Nercury
Copy link
Contributor

Nercury commented May 4, 2016

FYI, to cover all performance needs, users would be interested in these targets:

arm

This target is guaranteed to work on all arm abi android devices

arm-v7a

This target is guaranteed to work on all arm-v7a abi android devices

  • Thumb-2
  • VFPv3-D16
  • Soft-float ABI calls (note that this still can benefit from fp instructions)

arm-v7a-hard

This target would need to be loaded dynamically based on runtime check, and would fall back to arm-v7a. I imagine this runtime check might be done on Java side when application is loading to choose the correct shared library.

  • Thumb-2
  • VFPv3-D32
  • Hard-float ABI calls (question if and how rust can do it)
  • NEON

Most of this info comes from this source

@nagisa
Copy link
Member

nagisa commented May 4, 2016

arm-v7a

I think the float abi should be hard-float by default. It also seems to me like adding a different target for soft-float/hard-float distinction is not very interesting, because one can enable this by using some codegen flag when using the other target.

@Nercury
Copy link
Contributor

Nercury commented May 4, 2016

@nagisa I am re-reading armeabi-v7a (armeabi-v7a-hard) section, looks like it should be ok to enable Hard-float ABI calls on arm-v7a, as long as all linked binaries are using it.

However, latest docs do not mention this, except

The armeabi-v7a ABI uses the -mfloat-abi=softfp switch to enforce the rule that the compiler must pass all double values in core register pairs during function calls, instead of dedicated floating-point ones.

Which leads me to conclusion that we should use the same setting for armeabi-v7a.

I agree that separate target for hard(er) float does not look practical.

@alexcrichton
Copy link
Member

The tools team discussed this issue during triage yesterday, and the decision was to:

  1. Add an armv7-linux-androideabi target corresponding to the armeabi-v7a official ABI
  2. Eventually change arm-linux-androideabi to correspond to the armeabi official ABI
  3. Bring other targets (e.g. i686-linux-android) in line with the official ABIs

Along those lines, @Nercury could the new target be armv7 instead of armv7-a, and I'm curious what the purpose of a -hard target would be? That's not currently mentioned in the list of official ABIs, but in terms of Rust naming we'd probably call that armv7-linux-androideabihf.

@alexcrichton
Copy link
Member

It was also thought that we may not want to change the current arm-linux-androideabi target for a cycle or two to ensure we have enough time to message to everyone about the change.

@ollie27
Copy link
Member

ollie27 commented May 4, 2016

Can we even support armeabi as it is ARMv5 which I thought didn't have proper atomic support?

@alexcrichton
Copy link
Member

Yes, LLVM has fallbacks which we can rely on.

@Nercury
Copy link
Contributor

Nercury commented May 4, 2016

Along those lines, @Nercury could the new target be armv7

Yes, I changed latest name to be armv7, because android's LLVM configured for arm-v7a produces this header:

target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
target triple = "armv7-none-linux-android"

Which contains armv7, not armv7a.

I'm curious what the purpose of a -hard target would be

I imagined to make use of rust std lib compiled with this option. However it is removed in latest android abi docs so I don't think we will make use of it.

@alexcrichton
Copy link
Member

Yeah we can enable the "7a" features through LLVM features I think rather than having to communicate it as part of the triple name

bors added a commit that referenced this issue May 8, 2016
Add armv7-linux-androideabi target

This PR adds `armv7-linux-androideabi` target that matches `armeabi-v7a` Android ABI, ~~downscales `arm-linux-androideabi` target to match `armeabi` Android ABI~~ (TBD later if needed).

This should allow us to get the best performance from every [Android ABI level](http://developer.android.com/ndk/guides/abis.html).

Currently existing target `arm-linux-androideabi` started gaining features out of the supported range of [android `armeabi`](http://developer.android.com/ndk/guides/abis.html). While android compiler does not use a different target for later supported `armv7` architecture, it has distinct ABI name `armeabi-v7a`. We decided to add rust target `armv7-linux-androideabi` to match it.

Note that `NEON`, `VFPv3-D32`, and `ThumbEE` instruction sets are not added, because not all android devices are guaranteed to support all or some of these, and [their availability should be checked at runtime](http://developer.android.com/ndk/guides/abis.html#v7a).

~~This reduces performance of existing `arm-linux-androideabi` and may make it _much_ slower (we are talking more than order of magnitude in some random ad-hoc fp benchmark that I did).~~

Part of #33278.
@alexcrichton
Copy link
Member

This is done, and we now have nightlies, so closing.

@learnopengles
Copy link

Hi team, I know this is closed but it seems steps 2 and 3 aren't yet implemented. Should we reopen this or open new tracking issues?

romanb pushed a commit to wireapp/cryptobox-jni that referenced this issue Jan 24, 2017
  * Remove default paths for NDK standalone toolchains. Error if
    they are needed by a target and undefined.
  * Use the newer armv7-linux-androideabi Rust target, since we are
    only targeting armv7. See
    rust-lang/rust#33278.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants