riscv: Support RMW when zaamo enabled (even when unsafe-assume-single-core disabled)

[taiki-e]

Addresses #123 (comment)

cc @romancardenas

[romancardenas]

Thanks for this contribution! It will improve my life significantly

[romancardenas]

Hi, @taiki-e !

I'm trying out your latest work and wanted to share with you the actual status.

With rust version 1.83.0-nightly (363ae4188 2024-09-24)

When not enabling any portable-atomic feature and compiling it as follows:

RUSTFLAGS="-C target-feature=+zaamo" cargo build --target riscv32imc-unknown-none-elf

My project builds successfully. However, I get the following warning:

warning: unknown and unstable feature specified for `-Ctarget-feature`: `zaamo`
  |
  = note: it is still passed through to the codegen backend, but use of this feature might be unsound and the behavior of this feature can change in the future
  = help: consider filing a feature request

warning: `e310x` (lib) generated 1 warning
warning: `e310x-hal` (lib) generated 1 warning (1 duplicate)

When enabling the force-amo feature and compiling it as follows:

cargo build --target riscv32imc-unknown-none-elf

I get the following build error:

error: `portable_atomic_force_amo` cfg (`force-amo` feature) may only be used together with `portable_atomic_unsafe_assume_single_core` cfg (`unsafe-assume-single-core` feature)
   --> /Users/rcardenas/.cargo/git/checkouts/portable-atomic-1ec81f4f47ac5a29/23f8fd5/src/lib.rs:408:1
    |
408 | / compile_error!(
409 | |     "`portable_atomic_force_amo` cfg (`force-amo` feature) may only be used together with `portable_atomic_unsafe_ass...
410 | | );
    | |_^

error: could not compile `portable-atomic` (lib) due to 1 previous error

With rust version 1.83.0-stable

When not enabling any portable-atomic feature and compiling it as follows:

RUSTFLAGS="-C target-feature=+zaamo" cargo build --target riscv32imc-unknown-none-elf

My project builds successfully. However, I get tens of these warnings:

'+zaamo' is not a recognized feature for this target (ignoring feature)

Issue

Looks like the zaamo LLVM feature is not still supported in Rust. On the other hand, the force-amo feature still expects users to fully enable all atomic operations somehow (in this case, via critical sections but only for single core targets).

Proposal

What do you think about adding a new Cargo feature, let's say zaamo for instance, that allows users to have the same behavior as enabling the zaamo target feature but bypassing all these warnings? In this way, PACS and HALs of Zaamo targets could use native atomic operations without forcing users to depend on critical-section if they don't need it in their applications.

[taiki-e]

Thanks for the feedback!

First, I should talk about what the warnings are that you are encountering:

• The “unknown and unstable feature ..” warning comes from rustc and indicates that rustc is not aware of that feature. The results vary from feature to feature, but in zaamo's case the only problem is that cfg(target_feature = "..") and target_feature(enable = "..") do not work, and it is not a problem because portable-atomic has a fallback for this case.
• The "'+zaamo' is not a recognized feature .." warning in stable is from the old LLVM, in this case LLVM 18, and since LLVM 19+ recognizes zaamo, so this warning does not occur in Rust 1.82 (the current beta, will be released as stable in 2024-10-17) and later.
  Even if LLVM does not recognize it, it is not a problem because portable-atomic can use the fact that zaamo is a subset of a to generate the appropriate code.

Therefore, I believe that effectively the only problem that results is that the warnings are annoying or confusing.

• The correct way to handle the first warning is to make rustc aware of it, and I have opened a PR for that: rustc_target: Add RISC-V atomic-related features rust-lang/rust#130877
• There is no correct way to handle the second warning, but it is a warning that will disappear from the latest stable in a few weeks, and it is fairly common for warnings to occur on older versions (e.g., [lints] table), so I don't feel it is a big issue.

That said, there is already a working way to silence the warning in all situations, and that is to use --cfg=portable_atomic_target_feature="zaamo" instead of -C target-feature=+zaamo.
It is originally the mechanism used for the above fallback and is not currently considered a public API, but it is not planned to be changed and is also a suitable cfg for this use...

Since target_feature is unsafe in general (and so is zaamo), and given that there are already more than 10 target features in a similar situations, I'm not very positive about adding such a target feature as a Cargo feature.

[romancardenas]

Thank you so much for your explanation, it is way more clear for me.
I guess that, eventually, we will be able to use regular target-features in stable.

I adapted the e310x-hal to use this new feature and added a few docs here.

[taiki-e]

Published in 1.9.0.

Since this PR, there have been two related changes (both are included in 1.9.0):

• riscv: Provide all operations of AtomicBool when Zaamo enabled 9983a8b
  • AtomicBool is implemented in a way that does not require CAS on RISC-V, and I have reflected this.
• Document portable_atomic_target_feature="zaamo" cfg 02597c5
  • Documented portable_atomic_target_feature="zaamo" as a public API. Previously, it is not considered a public API.