Repository navigation
Re-enable outline-atomics by default on more AArch64 platforms - #164564
Conversation
Windows has a similar flag `/forceInterlockedFunctions`, which uses names such as `_InterlockedAdd64_rel`. This is a re-land of 1ed1b6e ("Enable `outline-atomics` by default on AArch64 Windows platforms").
Per LLVM commit c5e7e64 ("[AArch64][Clang][Linux] Enable out-of-line atomics by default.") [1], Clang enables these on Android. Thus, do the same in Rust. This is a re-land of c455903 ("Enable `outline-atomics` by default on AArch64 Android"). [1]: llvm/llvm-project@c5e7e649d537067de
Clang has done this by default since LLVM commit 1a963d3 ("[Driver] Make -moutline-atomics default for aarch64-fuchsia targets"), [1], so do the same here. This is a re-land of 21525f8 ("Enable `outline-atomics` by default on AArch64 Fuchsia"). [1]: llvm/llvm-project@1a963d3
Clang has recently started doing this, as of LLVM commit 5d774ec8d183
("[Driver] Enable outline atomics for OpenBSD/aarch64") [1]. Thus, do
the same here.
This is a re-land of 66c150c ("Enable `outline-atomics` by default
on AArch64 OpenBSD").
[1]: llvm/llvm-project@5d774ec
Many aarch64 targets without LSE in the baseline enable the `outline-atomics` feature, which uses runtime detection of LSE for its faster atomic ops. This provides nontrivial performance improvements on most hardware from the past decade, at a small cost to anything pre-LSE. This matches what Clang does [1]. This is a re-land of 383053e ("Enable `outline-atomics` by default on AArch64 FreeBSD"). [1]: llvm/llvm-project@e24f901
edb977e to
ec3b6ca
Compare
|
@bors try jobs=dist-aarch64-freebsd,dist-aarch64-msvc |
This comment has been minimized.
This comment has been minimized.
Re-enable outline-atomics by default on more AArch64 platforms try-job: dist-aarch64-freebsd try-job: dist-aarch64-msvc
|
These commits modify compiler targets. |
|
r? @chenyukang rustbot has assigned @chenyukang. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rustbot reroll |
|
I understand review capacity is limited, but is there a way we can get this landed soon, or restore and land #165212? We're unable to qualify new toolchains without this, so we'd like to see this go through ASAP. Is there something we could help with? |
|
For future reference you can always ask on Zulip, I did so at #t-compiler/help > Review request. For my understanding, why did this become necessary in Fuchsia? The inline versions should work fine, and you can mix and match. |
|
@bors r+ rollup |
|
We were hitting compilation failures like https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/rust_toolchain.core.arm64-release-subbuild/b8666562801022530657/overview, which I think should be accessible from outside Google. If not, it was errors like this where the outline atomic functions were undefined: We traced the issue down to the roll of compiler-builtins PR rust-lang/compiler-builtins#1354, which as far as we can tell ended up only compiling these functions if the |
|
It looks like those locations point to After this PR merges or if you set (unstable) |
…gjubilee Re-enable outline-atomics by default on more AArch64 platforms These originally landed in rust-lang#144938 and rust-lang#149633, then were reverted in rust-lang#151896 due to rust-lang#151486. As that issue has been resolved, re-enable them here. There is a lot of additional context at the original PR (rust-lang#144938) and the Zulip discussion leading up to it [#t-compiler > outline-atomics on non-Linux targets](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/outline-atomics.20on.20non-Linux.20targets/with/537964562).
…gjubilee Re-enable outline-atomics by default on more AArch64 platforms These originally landed in rust-lang#144938 and rust-lang#149633, then were reverted in rust-lang#151896 due to rust-lang#151486. As that issue has been resolved, re-enable them here. There is a lot of additional context at the original PR (rust-lang#144938) and the Zulip discussion leading up to it [#t-compiler > outline-atomics on non-Linux targets](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/outline-atomics.20on.20non-Linux.20targets/with/537964562).
Rollup of 13 pull requests Successful merges: - #163477 (Fancy paramenv accessors that communicate the access pattern better.) - #160593 (turn ABI-controlling target features into a target modifier) - #164896 (offload: pass the linked device image to the host pass) - #165468 (Remove return type from `BuilderMethods::store`) - #159374 (Lower #[no_mangle] to #[symbol_name]/#[link_name]) - #164564 (Re-enable outline-atomics by default on more AArch64 platforms) - #164623 (add next-solver regression tests for two fixed ices) - #164678 (add next-solver regression test for recursive opaque ICE under validate-mir) - #165429 (Add tests for `-Zcrate-attr` edition and position) - #165430 ([tiny] Show context for found assoc item in E0107) - #165456 (std: Fix build for the L4Re targets) - #165463 (Add BTreeMap::allocator and BTreeSet::allocator) - #165466 (Don't "exit" using `panic` on teeos)
Rollup of 15 pull requests Successful merges: - #163477 (Fancy paramenv accessors that communicate the access pattern better.) - #160593 (turn ABI-controlling target features into a target modifier) - #164896 (offload: pass the linked device image to the host pass) - #165468 (Remove return type from `BuilderMethods::store`) - #164564 (Re-enable outline-atomics by default on more AArch64 platforms) - #164623 (add next-solver regression tests for two fixed ices) - #164653 (Deprecate internal function `vec::from_elem`) - #164678 (add next-solver regression test for recursive opaque ICE under validate-mir) - #165387 (windows OpenOptionsExt examples: replace winapi by windows crate) - #165429 (Add tests for `-Zcrate-attr` edition and position) - #165430 ([tiny] Show context for found assoc item in E0107) - #165456 (std: Fix build for the L4Re targets) - #165463 (Add BTreeMap::allocator and BTreeSet::allocator) - #165466 (Don't "exit" using `panic` on teeos) - #165491 (Remove unused compiler types and associated helpers)
Rollup merge of #164564 - tgross35:outline-atomics, r=workingjubilee Re-enable outline-atomics by default on more AArch64 platforms These originally landed in #144938 and #149633, then were reverted in #151896 due to #151486. As that issue has been resolved, re-enable them here. There is a lot of additional context at the original PR (#144938) and the Zulip discussion leading up to it [#t-compiler > outline-atomics on non-Linux targets](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/outline-atomics.20on.20non-Linux.20targets/with/537964562).
|
First thanks for prioritizing this. And sorry for the belated response.
Yes. Outline atomics is the default in clang for a bit now, so it's set there. The recent change to the builtins disabled the ability to defer the atomics to compiler-rt version when building the toolchain, unless this feature is part of the triple.
It's important to us that we use a single copy of the builtins since we have arbitrarily mixed Rust and C++, and we typically expect them to work identically even in the presence of cross-language inlining. Our intent is always to have the code generation options set consistently between all of our toolchains, and for us to use the canonical versions of common runtime routines, like the builtins.
While there may be a workaround, we'd prefer not to have to manually add the compiler builtins to all of our targets, or communicate that to people building for the Fuchsia platform. This option seems like the first step of several we'll want to pursue to align these things up across all of our targets, and is the minimal thing that would get us new candidates w/o fairly large scale build system surgery that we expect to be rather temporary. |
These originally landed in #144938 and #149633, then were reverted in #151896 due to #151486. As that issue has been resolved, re-enable them here.
There is a lot of additional context at the original PR (#144938) and the Zulip discussion leading up to it #t-compiler > outline-atomics on non-Linux targets.