compiler: Make +fix-cortex-a53-835769 a default feature for aarch64 fuchsia - #161886
Conversation
…uchsia This matches the default target features for aarch64 fuchsia emitted by clang.
|
These commits modify compiler targets. |
|
r? @jieyouxu rustbot has assigned @jieyouxu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
cc @ilovepi to make sure this is the right spelling of this |
|
This should be the right way to spell this on Fuchsia. We should also consider just always setting this in LLVM if its targeting a Fuchsia triple. We may also need to adjust the build Rules on the Fuchsia side when this lands, since its likely to change the ABI of the core snd std libraries. I have a vague memory that ordering may also have been important, so we should check that all the things that care (specifically zircon's build rules) use a compatible ordering. |
There was a problem hiding this comment.
(This seems sensible, but I'll wait for a target maintainer to double-check EDIT: sorry, I think @ilovepi was meant to be one of the target maintainers?)
| let mut base = base::fuchsia::opts(); | ||
| base.cpu = "generic".into(); | ||
| base.features = "+v8a,+crc,+aes,+sha2,+neon".into(); | ||
| base.features = "+v8a,+crc,+aes,+sha2,+neon,+fix-cortex-a53-835769".into(); |
There was a problem hiding this comment.
Is there more context for this? Would be helpful if the PR description could be slightly expanded to backlink to relevant bugs/context. I did a basic search and found:
- ld.lld: error: unknown argument '--fix-cortex-a53-835769' when trying to compile rust on aarch64 llvm/llvm-project#155116
- https://gcc.gnu.org/legacy-ml/gcc-patches/2014-10/msg00906.html
- https://reviews.llvm.org/D5721
In particular for the gcc/llvm patch
Some early revisions of the Cortex-A53 have an erratum (835769) whereby it is possible for a 64-bit multiply-accumulate instruction in AArch64 state to generate an incorrect result. The details are quite complex and hard to determine statically, since branches in the code may exist in some circumstances, but all cases end with a memory (load, store, or prefetch) instruction followed immediately by the multiply-accumulate operation.
The safest work-around for this issue is to make the compiler avoid emitting multiply-accumulate instructions immediately after memory instructions and the simplest way to do this is to insert a NOP.
This patch implements such work-around. The work-around is only enabled when specifying the clang command line option -mfix-cortex-a53-835769 or the llvm backend option -aarch64-fix-cortex-a53-835769.
The work-around code generation is not enabled by default.
Presumably this PR here changes the default target features so that the workaround is enabled-by-default for the aarch64 fuchsia target?
cc @erickt @Nashenas88 (target maintainers, I don't know if Fuchsia has different target maintainers or sth)
There was a problem hiding this comment.
Fushsia's C++ code already always adds this in clang (see https://github.com/llvm/llvm-project/blob/1a315dc409bd5eec1c4df57ae9225b518c1c265a/clang/lib/Driver/ToolChains/Fuchsia.cpp#L94). It's been an oversight that it wasn't happening in Rust too. This is unfortunate in that it likely has prevented cross language inlining in our LTO builds. We've been introducing Rust into our kernel and the build changes there are where we first noticed the incompatibility.
| let mut base = base::fuchsia::opts(); | ||
| base.cpu = "generic".into(); | ||
| base.features = "+v8a,+crc,+aes,+sha2,+neon".into(); | ||
| base.features = "+v8a,+crc,+aes,+sha2,+neon,+fix-cortex-a53-835769".into(); |
There was a problem hiding this comment.
This should be the right way to spell this on Fuchsia. We should also consider just always setting this in LLVM if its targeting a Fuchsia triple.
This sounds sensible to me. I think it's fine if we do it on the Rust side if you'd like to get this enabled-by-default sooner, but it sounds like the more sane default over the longer term is to change the LLVM side to enable this by default (and if there's some obscure reason to not want this, provide an opt-out).
There was a problem hiding this comment.
Oh hm, I think the fuchsia ping group / target maintainers never got updated, there's
- Update fuchsia.toml to reflect team changes team#1614
- https://doc.rust-lang.org/nightly/rustc/platform-support/fuchsia.html
@ilovepi do you happen to know what's the actual intended group of target maintainers for the Fuchsia targets?
There was a problem hiding this comment.
I think its roughly our Toolchain team. That includes @PiJoules @petrhosek @erickt @zeroomega @gulfemsavrun @Prabhuk @mysterymath and me.
There was a problem hiding this comment.
@erickt can you confirm if there are more folks? We should probably also land the referenced PR w/ the full set of team members.
There was a problem hiding this comment.
Also FYI -- Fuchsia's current test job was disabled some ago due to hitting fuchsia server rate limits:
rust/src/ci/github-actions/jobs.yml
Lines 367 to 376 in 17fd5b8
I'm not sure if this is still intended (i.e. the server rate limits is annoying to solve), or if this fell off the radar. But just FYI that in terms of the integration test job, Fuchsia hasn't been running in CI for a while.
Rollup merge of #161886 - PiJoules:fix-cortex-a53-835769, r=jieyouxu compiler: Make +fix-cortex-a53-835769 a default feature for aarch64 fuchsia This matches the default target features for aarch64 fuchsia emitted by clang.
This matches the default target features for aarch64 fuchsia emitted by clang.