Add armv7neon variants of armv7 android, gnueabihf and musleabihf targets #49902
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Is there perhaps a convention on Linux for how these targets are named (if at all?). I think we've lifted most of our naming from Debian historically. Additionally, is this important enough that the standard library itself needs to be recompiled? Or does |
|
RPM defines them here: But it also tends toward a plain |
There was a problem hiding this comment.
Ok thanks @cuviper! Sounds like these are good names if we'd like to add them then.
| abi_blacklist: super::arm_base::abi_blacklist(), | ||
| .. base | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Could this use armv7_linux_androideabi as a base and only modify the features?
There was a problem hiding this comment.
Er actually, could this be done for the variants below as well?
Prior to @cuviper's comment, I wasn't aware of any.
As far as I'm aware, Debian (or notable Debian derivatives like Ubuntu) don't have ARMv7+NEON as a distinct architecture. This is, of course, a significant part of the problem here.
I made this PR at this time based on @gnzlbg's remarks in the related issues suggesting that More speculatively, I'm hoping that I'll be able to speed up UTF-8 validation using NEON on ARMv7 like I was able to speed up UTF-8 validation using SSE2 on x86 and x86_64 and using NEON on aarch64. Once the ARMv7+NEON case is done (assuming it's a success), I was planning to upstream the code to the Rust standard library, because it's bad for the faster UTF-8 validation code to be elsewhere (currently in encoding_rs). So if NEON indeed ends up making UTF-8 validation faster and gets upstreamed, it's relevant to compile the standard library in a way that actually makes use of it. (But, to be clear, I haven't done the ARMv7+NEON work yet, so I don't yet know if it is going to be a win.)
Thank you. I was unaware of this precedent.
The fourth component of the "triple" is the ABI and which is not changing here, at least not at this time. Consistency with |
These are two different independent issues and both must be resolved:
|
|
Ok! I'm tempted to land this in light of that info (thanks @hsivonen!). I wonder though if we could hold off on shipping an Android target until there's some solid wins in libstd though? (aka basically tweak this to only add the definitions) |
parched
reviewed
Apr 14, 2018
|
|
||
| pub fn target() -> TargetResult { | ||
| let mut base = super::android_base::opts(); | ||
| base.features = "+v7,+thumb-mode,+thumb2,+vfp3,+d16,+neon".to_string(); |
There was a problem hiding this comment.
+v7 and +thumb2 are redundant given the armv7 in the llvm_target. +d16 should also be removed given NEON requires 32 registers (see ARM®
Architecture Reference Manual A1-33).
| .get_mut(&LinkerFlavor::Gcc).unwrap().push("-march=armv7-a".to_string()); | ||
|
|
||
| Ok(Target { | ||
| llvm_target: "armv7-none-linux-android".to_string(), |
There was a problem hiding this comment.
Shouldn't it be unknown not none?
| @@ -0,0 +1,40 @@ | |||
| // Copyright 2016 The Rust Project Developers. See the COPYRIGHT | |||
There was a problem hiding this comment.
IMO it should probably start thumbv7 not armv7 because it generates Thumb code not Arm code (related #44722)
|
Ping from triage @alexcrichton! What's the status on this? |
|
I'm currently confused about what I should do about the |
|
The only difference of this from the existing armv7 targets is enabling neon, right? In that sense I'd personally be in favor of sticking with our current naming convention and just tacking on |
As the changeset is currently written, yes. However, my goals a two-fold:
The difference in |
|
@hsivonen for the latter wouldn't a custom compilation locally with |
Unclear. Does a difference in In any case, having the same machine identifier for Android and GNU/Linux use different instruction sets leading to (presumably) different performance characteristics seems on surface like a gotcha that we shouldn't have. |
Though for instruction set extensions this is already the case for |
Why not just add Arm and Thumb versions of both? AFAIK |
|
Hm well so at some point there's basically a never-ending matrix of targets we could ship, especially with respect to ARM. I'd rather be far more strategic about our options and selectively offer popular platforms rather than adding the matrix of everything. It sounds like @hsivonen you've got a case for libstd itself being compiled with this support, in which case I'm ok adding this as a target. I would prefer to not add four new targets here. I don't think that's worth theoretical consistency, I think we should stick with what's needed. |
|
Would it be better idea to name the targets |
If Debian armhf uses thumb, does anyone actually want to use the non-thumb ARMv7 instructions? Is there a reference showing that Debian uses thumb for C and C++? Making the GNU/Linux targets thumb, too, and citing Debian as justification would solve my issue nicely. |
|
@hsivonen This is where the default for armhf is set https://sources.debian.org/src/gcc-7/7.3.0-16/debian/rules.defs/#L395 I believe. From memory though, some other distros default to Arm though, like Arch Linux I think, but I'd have to check. |
|
☔ The latest upstream changes (presumably #50228) made this pull request unmergeable. Please resolve the merge conflicts. |
|
What are the next steps here? Are we still trying to figure what exactly these targets are? If so could custom target specifications be used to iterate out of tree? |
|
I think the next step is to figure out 1) why the Android target uses @makotokato, what's the story behind the change to Do the armv7 versions of Ubuntu and Fedora use thumb? Why or why not? |
This is android's ABI document. https://developer.android.com/ndk/guides/abis#v7a armeabi-v7a (armv7-linux-androideabi) supports thumb-2 and we would like to build all rust library (including standard library) by thumb2 for binary size and reduce interwork calls.
As long as I know, it depends on ABI. Softfp EABI (gnueabi) builds ARM32 as default, but Hardfp EABI (gnueabihf) builds Thumb2 as default. |
Thank you! Still one complication: Using the RPM naming |
|
@bors r- |
|
I pushed the rebase just now, but it still fails (after removing cargo caches, rust build directories and the Docker image and rebuilding everything): Any ideas what I should try next? |
|
📌 Commit 072fdae has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 072fdae with merge 61534c3a7ca201b7cb58aac0c0624e01d09e1ff6... |
|
💔 Test failed - status-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-review
kennytm
added
S-waiting-on-author
|
@bors r- Wrongly rescheduled |
|
☔ The latest upstream changes (presumably #54301) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping from triage, @hsivonen: It looks like your PR failed on travis and need a rebuild. |
|
I need to fix the build for compiler-rt next, but I haven't had time to work on this recently for reasons unrelated to this issue. I haven't forgotten about this. |
|
Ping from triage @hsivonen! I'm closing this due to inactivity per the triage procedure. Thank you for your contribution and please feel free to (re-)open this or another PR in the future when you're able to work on this again. |
TimNN
added
S-inactive
|
@hsivonen GitHub does not allow reopening PRs after they have been force pushed, so it's necessary to create a new PR instead. |
Thanks. Created #56947. |
pietroalbini
added a commit
to pietroalbini/rust
that referenced
this pull request
Dec 19, 2018
Add targets thumbv7neon-linux-androideabi and thumbv7neon-unknown-linux-gnueabihf These two targets enable both thumb-mode and NEON for ARMv7 CPUs. This another attempt at rust-lang#49902, which cannot be reopened. Between that PR and this one, some subrepos with C code whose build systems were failing went away.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds the targets requested in #49897. It doesn't deal with the part of getting the standard library builds distributed via rustup.rs.