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

Add armv7-linux-androideabi target #33414

Merged
merged 2 commits into from
May 8, 2016
Merged

Add armv7-linux-androideabi target #33414

merged 2 commits into from
May 8, 2016

Conversation

Nercury
Copy link
Contributor

@Nercury Nercury commented May 4, 2016

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.

Currently existing target arm-linux-androideabi started gaining features out of the supported range of android armeabi. 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.

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.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Nercury
Copy link
Contributor Author

Nercury commented May 4, 2016

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned aturon May 4, 2016
@@ -12,7 +12,7 @@ use target::Target;

pub fn target() -> Target {
let mut base = super::android_base::opts();
base.features = "+v7,+vfp3,+d16".to_string();
base.features = "+v6".to_string();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this target temporarily stay where it is today for now? We'd like to give a window of time for projects to move over to the armv7 target before we downgrade the arm one. I don't believe we've received a request yet to use arm instead of armv7, so it's not super pressing to do so just yet

@alexcrichton
Copy link
Member

Thanks @Nercury! Just one nit about not changing arm for now (we can do that once armv7 has nightlies and such), but other than that looks good to me

@Nercury
Copy link
Contributor Author

Nercury commented May 5, 2016

Changes to current arm removed.

I also saw few tests using current arm target. Are we going to move or duplicate them later for armv7?

@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label May 5, 2016
@alexcrichton
Copy link
Member

@bors: r+ a9a5a52be5d077e402b7cc029b62d8b5c44c1519

I also saw few tests using current arm target. Are we going to move or duplicate them later for armv7?

Probably, yeah.

@bors
Copy link
Contributor

bors commented May 7, 2016

☔ The latest upstream changes (presumably #33228) made this pull request unmergeable. Please resolve the merge conflicts.

@Nercury
Copy link
Contributor Author

Nercury commented May 7, 2016

r? @alexcrichton

(I am not sure if this comment was necessary)

@alexcrichton
Copy link
Member

@bors: r+ ca03b81

@bors
Copy link
Contributor

bors commented May 8, 2016

⌛ Testing commit ca03b81 with merge ebe6da3...

bors added a commit that referenced this pull request 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.
@bors bors merged commit ca03b81 into rust-lang:master May 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants