-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
[RISCV] Disable Atomics on all Non-A RISC-V targets #66548
Conversation
@bors: r+ rollup |
📌 Commit ca42c25 has been approved by |
After a discussion with @asb, we have established we don't need to update the |
so is this one looking like it will go in? |
… r=alexcrichton [RISCV] Disable Atomics on all Non-A RISC-V targets In a `TargetOptions` configuration, `max_atomic_width: None` causes `max_atomic_width()` to return `Some(target_pointer_width)`. So, contrary to assumptions, `max_atomic_width: None` means you do have atomic support! RISC-V's rv32i and rv32imc do not have architectural support for atomic memory accesses of any size, because they do not include the `A` architecture extension. This means the values in the target definition should be `Some(0)`. This bug has been observed via a build failure with oreboot/oreboot#191, where LLVM was still generating libcalls for atomic operations. According to rust-lang/compiler-builtins, "Rust only exposes atomic types on platforms that support them, and therefore does not need to fall back to software implementations." - so this PR tries to bring rustc inline with this decision. This commit also removes the outdated bug link, which references a now irrelevant GCC bug. I will likely also have to revisit the `min_atomic_width` of all the RISC-V targets so they are correct and match what the hardware is capable of (which is more restricted than one might imagine). r? @alexcrichton
Rollup of 8 pull requests Successful merges: - #65665 (Update Source Code Pro and include italics) - #66478 (rustc_plugin: Remove the compatibility shim) - #66497 (Fix #53820) - #66526 (Add more context to `async fn` trait error) - #66532 (Generate DWARF address ranges for faster lookups) - #66546 (Remove duplicate function) - #66548 ([RISCV] Disable Atomics on all Non-A RISC-V targets) - #66553 (remove HermitCore leftovers from sys/unix) Failed merges: r? @ghost
AFAICT atomic loads and stores are supported even without the A extension for as long as they are aligned. There is also a fence instruction in thr base isa supporting both SeqCst and TSO orderings. Only read modify write and (compare) exchange require the A extension. As rust doesn't fallback to software implementations there is no need to force plain atomic loads and stores to be libcalls to sync with the operations riscv doesn't support natively. Those operations simply aren't exposed to the user. |
This roughly reverts PR rust-lang#66548 Atomic "CAS" are still disabled for targets without the *“A” Standard Extension for Atomic Instructions*. However this extension only adds instructions for operations more complex than simple loads and stores, which are always atomic when aligned. In the [Unprivileged Spec v. 20191213](https://riscv.org/technical/specifications/) section 2.6 *Load and Store Instructions* of chapter 2 *RV32I Base Integer Instruction Set* (emphasis mine): > Even when misaligned loads and stores complete successfully, > these accesses might run extremely slowly depending on the implementation > (e.g., when implemented via an invisible trap). Further-more, whereas > **naturally aligned loads and stores are guaranteed to execute atomically**, > misaligned loads and stores might not, and hence require > additional synchronization to ensure atomicity. Unfortunately PR rust-lang#66548 did not provide much details on the bug that motivated it, but rust-lang#66240 and rust-lang#85736 appear related and happen with targets that do have the A extension.
Re-enable atomic loads and stores for all RISC-V targets This roughly reverts PR rust-lang#66548 Atomic "CAS" are still disabled for targets without the *“A” Standard Extension for Atomic Instructions*. However this extension only adds instructions for operations more complex than simple loads and stores, which are always atomic when aligned. In the [Unprivileged Spec v. 20191213](https://riscv.org/technical/specifications/) section 2.6 *Load and Store Instructions* of chapter 2 *RV32I Base Integer Instruction Set* (emphasis mine): > Even when misaligned loads and stores complete successfully, these accesses might run extremely slowly depending on the implementation (e.g., when implemented via an invisible trap). Further-more, whereas **naturally aligned loads and stores are guaranteed to execute atomically**, misaligned loads and stores might not, and hence require additional synchronization to ensure atomicity. Unfortunately PR rust-lang#66548 did not provide much details on the bug that motivated it, but rust-lang#66240 and rust-lang#85736 appear related and happen with targets that do have the A extension.
In a
TargetOptions
configuration,max_atomic_width: None
causesmax_atomic_width()
to returnSome(target_pointer_width)
. So, contrary to assumptions,max_atomic_width: None
means you do have atomic support!RISC-V's rv32i and rv32imc do not have architectural support for atomic memory accesses of any size, because they do not include the
A
architecture extension. This means the values in the target definition should beSome(0)
.This bug has been observed via a build failure with oreboot/oreboot#191, where LLVM was still generating libcalls for atomic operations. According to rust-lang/compiler-builtins, "Rust only exposes atomic types on platforms that support them, and therefore does not need to fall back to software implementations." - so this PR tries to bring rustc inline with this decision.
This commit also removes the outdated bug link, which references a now irrelevant GCC bug.
I will likely also have to revisit the
min_atomic_width
of all the RISC-V targets so they are correct and match what the hardware is capable of (which is more restricted than one might imagine).r? @alexcrichton