Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf #114499

taiki-e · 2023-08-05T05:11:22Z

As said in #98333 (comment), forced-atomics target feature is also needed to enable atomic load/store on these targets (otherwise, libcalls are generated): https://godbolt.org/z/433qeG7vd

~~This PR is currently marked as a draft because:~~

~~forced-atomics target feature is currently broken (Use of RiscV "+forced-atomics" target feature triggers hard-float ABI for symbols.o #114153).~~ EDIT: Fixed
forced-atomics target feature has been added in LLVM 16 (llvm/llvm-project@f5ed0cb), but the current minimum LLVM version is 15. In LLVM 15, the atomic load/store of these targets generates libcalls anyway. EDIT: LLVM 15 has been dropped

Depending on the policy on the minimum LLVM version for these targets, this may be blocked until the minimum LLVM version is increased to 16.

r? @Amanieu

taiki-e · 2023-08-05T05:15:31Z

@rustbot label -S-waiting-on-review +S-blocked

rust-lang/rust#98333 broke RISC-V targets without A-extension. This will be fixed by rust-lang/rust#114497 or rust-lang/rust#114499. ``` = note: rust-lld: error: undefined symbol: __atomic_load_4 >>> referenced by mod.rs:1242 (/rustc/eb088b8b9d98f1af1b0e61bbdcd8686e1b0db7b6/library/core/src/num/mod.rs:1242) >>> compiler_builtins-d066fd6ed508b6b5.compiler_builtins.b1b28d926042a9f7-cgu.004.rcgu.o:(compiler_builtins::mem::memcpy::he6d5500b219c1d3d) in archive /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/riscv32i-unknown-none-elf/lib/libcompiler_builtins-d066fd6ed508b6b5.rlib ```

rust-lang/rust#98333 broke RISC-V targets without A-extension. This will be fixed by rust-lang/rust#114497 or rust-lang/rust#114499. ``` = note: rust-lld: error: undefined symbol: __atomic_load_4 >>> referenced by uint_macros.rs:1230 (/home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/num/uint_macros.rs:1230) >>> compiler_builtins-a15f77f0f647aa99.compiler_builtins.eedcbccd0d1b9b88-cgu.1.rcgu.o:(compiler_builtins::mem::memcpy::hedd00e0c59d2a943) in archive /home/runner/work/portable-atomic/portable-atomic/target/riscv32im-unknown-none-elf/debug/deps/libcompiler_builtins-a15f77f0f647aa99.rlib ```

MabezDev · 2023-11-06T15:39:10Z

Just an FYI the check has moved to

rust/src/bootstrap/src/core/build_steps/llvm.rs

Line 567 in e1fcecb

panic!("\n\nbad LLVM version: {version}, need >=15.0\n\n")

and is still currently at a minimum version of 15.

MabezDev · 2023-11-06T23:40:27Z

Depending on the policy on the minimum LLVM version for these targets, this may be blocked until the minimum LLVM version is increased to 16.

Is there a policy defined somewhere? If there isn't, in this case, it might be better to merge this now, as there is no downside to end users that I am aware of.

We currently have quite a few issues moving to portable-atomic in the bare-metal esp riscv ecosystem because of these hardcoded defines: https://github.com/taiki-e/portable-atomic/blob/bc457f235b7dac1e64b4015f7c1b79d4897a8ae4/no_atomic.rs#L77-L79. They conflict with various atomic emulation methods we had been previously using with atomic_polyfil. We're trying to work around these, but I believe having this merged would fix most of them.

taiki-e · 2023-11-07T01:32:05Z

hardcoded defines: https://github.com/taiki-e/portable-atomic/blob/bc457f235b7dac1e64b4015f7c1b79d4897a8ae4/no_atomic.rs#L77-L79.

The stuff about riscv, msp430, and avr on that list will be ignored, portable-atomic provides lock-free atomic load/store¹ by default for those targets (to be precise: RISC-V with not(target_has_atomic = "ptr"), all msp430, all avr) regardless of what is written there.

https://github.com/taiki-e/portable-atomic/blob/c5751dcff2185a46a987542f59a6f73dfc9044ed/src/imp/riscv.rs

For msp430 and avr, RMWs are also provided by default. ↩

bors · 2023-11-08T21:15:50Z

☔ The latest upstream changes (presumably #117716) made this pull request unmergeable. Please resolve the merge conflicts.

MabezDev · 2023-11-15T14:13:51Z

@taiki-e @Amanieu what are your thoughts on rebasing and getting this merged in its current state? All toolchains from rustup are shipping with LLVM16, older compilers built with LLVM15 will continue to work (without this feature). The only possible breakage is a custom build of modern rust + LLVM15, and even then that only applies to the riscv32 targets - seems like an unlikely source of breakage, with a huge upside for riscv32 developers.

Dirbaio · 2023-11-15T14:17:14Z

ping!

Could we merge this? It's causing quite a bit of annoyances in the embedded ecosystem: porting crates to riscv32imc requires doing hacks to polyfill atomic loads/stores that the hardware is already capable of.

Can we merge this before dropping LLVM 15? It seems LLVM silently ignores unknown target features, so code using atomics does compile with LLVM 15. It might generate broken code (with libcalls) but it doesn't break more code, since code using atomics simply didn't compile at all before this PR, so it's a net improvement, no regressions.
If not, can we drop LLVM 15 now? What's the policy?

taiki-e · 2023-11-15T14:35:32Z

Rebased.

I would like to merge this if there is no problem with the policy on the minimum LLVM version for these targets, but I'm not a maintainer, so I don't know about that.

It might generate broken code (with libcalls) but it doesn't break more code, since code using atomics simply didn't compile at all before this PR, so it's a net improvement, no regressions.

No, this causes a regression with LLVM 15 because compiler-builtins (and maybe libcore too) has code that depends on cfg(target_has_atomic_load_store). This is an error that actually occurred when #98333 (already reverted) was merged: taiki-e/semihosting@472e58a

rustbot · 2023-11-15T15:44:26Z

These commits modify compiler targets.
(See the Target Tier Policy.)

Dirbaio · 2023-11-15T19:03:36Z

#117947 drops llvm 15.

MabezDev · 2023-11-28T00:06:25Z

#117947 has been merged, which unblocks this PR.

Dirbaio · 2023-11-28T00:11:40Z

compiler/rustc_codegen_llvm/src/llvm_util.rs

@@ -507,6 +507,8 @@ pub(crate) fn global_llvm_features(sess: &Session, diagnostics: bool) -> Vec<Str
            .features
            .split(',')
            .filter(|v| !v.is_empty() && backend_feature_name(v).is_some())
+            // Drop +forced-atomics feature introduced in LLVM 16.
+            .filter(|v| *v != "+forced-atomics" || get_version() >= (16, 0, 0))


this can be removed, LLVM16 is required now.

Dirbaio · 2023-11-28T11:19:56Z

that was an instant review! ⚡

bors · 2023-11-29T15:48:30Z

⌛ Testing commit b25fa9a with merge 2936f1e...

Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf As said in rust-lang#98333 (comment), `forced-atomics` target feature is also needed to enable atomic load/store on these targets (otherwise, libcalls are generated): https://godbolt.org/z/433qeG7vd ~~This PR is currently marked as a draft because:~~ - ~~`forced-atomics` target feature is currently broken (rust-lang#114153 EDIT: Fixed - ~~`forced-atomics` target feature has been added in LLVM 16 (llvm/llvm-project@f5ed0cb), but the current minimum LLVM version [is 15](https://github.com/rust-lang/rust/blob/90f0b24ad3e7fc0dc0e419c9da30d74629cd5736/src/bootstrap/llvm.rs#L557). In LLVM 15, the atomic load/store of these targets generates libcalls anyway.~~ EDIT: LLVM 15 has been dropped Depending on the policy on the minimum LLVM version for these targets, this may be blocked until the minimum LLVM version is increased to 16. r? `@Amanieu`

bors · 2023-11-29T15:51:53Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-11-29T15:51:56Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

taiki-e · 2023-11-29T15:58:58Z

  Step 1/18 : FROM ubuntu:22.04
  error parsing HTTP 429 response body: invalid character 'T' looking for beginning of value: "Too Many Requests (HAP429).\n"

It seems to be a docker-related network error (unrelated to this PR's change)

Amanieu · 2023-11-29T16:18:04Z

@bors retry

bors · 2023-11-29T16:21:53Z

⌛ Testing commit b25fa9a with merge b4bdc80...

Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf As said in rust-lang#98333 (comment), `forced-atomics` target feature is also needed to enable atomic load/store on these targets (otherwise, libcalls are generated): https://godbolt.org/z/433qeG7vd ~~This PR is currently marked as a draft because:~~ - ~~`forced-atomics` target feature is currently broken (rust-lang#114153 EDIT: Fixed - ~~`forced-atomics` target feature has been added in LLVM 16 (llvm/llvm-project@f5ed0cb), but the current minimum LLVM version [is 15](https://github.com/rust-lang/rust/blob/90f0b24ad3e7fc0dc0e419c9da30d74629cd5736/src/bootstrap/llvm.rs#L557). In LLVM 15, the atomic load/store of these targets generates libcalls anyway.~~ EDIT: LLVM 15 has been dropped Depending on the policy on the minimum LLVM version for these targets, this may be blocked until the minimum LLVM version is increased to 16. r? `@Amanieu`

bors · 2023-11-29T16:23:41Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-11-29T17:11:08Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

MabezDev · 2023-11-29T17:30:50Z

Still network related by the looks of it :/

compiler-errors · 2023-11-29T20:24:41Z

@bors retry

bors · 2023-11-30T00:10:14Z

⌛ Testing commit b25fa9a with merge c9c760f...

bors · 2023-11-30T02:07:55Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing c9c760f to master...

rust-timer · 2023-11-30T03:29:56Z

Finished benchmarking commit (c9c760f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.01s -> 672.348s (-0.39%)
Artifact size: 313.40 MiB -> 313.40 MiB (-0.00%)

rustbot assigned Amanieu Aug 5, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 5, 2023

taiki-e mentioned this pull request Aug 5, 2023

Revert #98333 "Re-enable atomic loads and stores for all RISC-V targets" #114497

Merged

rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2023

taiki-e force-pushed the riscv-forced-atomics branch from 4227d24 to d554090 Compare August 8, 2023 12:22

taiki-e force-pushed the riscv-forced-atomics branch from d554090 to b0346d5 Compare August 20, 2023 06:27

This comment has been minimized.

Sign in to view

taiki-e mentioned this pull request Oct 5, 2023

document when atomic loads are guaranteed read-only #115577

Merged

taiki-e force-pushed the riscv-forced-atomics branch from b0346d5 to ad5d98a Compare November 15, 2023 14:27

This comment has been minimized.

Sign in to view

taiki-e force-pushed the riscv-forced-atomics branch from ad5d98a to 49aa5da Compare November 15, 2023 14:48

taiki-e marked this pull request as ready for review November 15, 2023 15:44

Dirbaio reviewed Nov 28, 2023

View reviewed changes

Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf

b25fa9a

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 29, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 29, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 29, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 29, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 30, 2023

bors merged commit c9c760f into rust-lang:master Nov 30, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 30, 2023

taiki-e deleted the riscv-forced-atomics branch November 30, 2023 02:11

bugadani mentioned this pull request Nov 30, 2023

Figure out if portable-atomic and the trap handler can coexist esp-rs/esp-hal#965

Closed

Dirbaio mentioned this pull request Dec 3, 2023

remove portable-atomic embassy-rs/embassy#2242

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf #114499

Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf #114499

taiki-e commented Aug 5, 2023 •

edited

taiki-e commented Aug 5, 2023

This comment has been minimized.

MabezDev commented Nov 6, 2023

MabezDev commented Nov 6, 2023

taiki-e commented Nov 7, 2023 •

edited

bors commented Nov 8, 2023

MabezDev commented Nov 15, 2023

Dirbaio commented Nov 15, 2023

taiki-e commented Nov 15, 2023

This comment has been minimized.

rustbot commented Nov 15, 2023

Dirbaio commented Nov 15, 2023 •

edited

MabezDev commented Nov 28, 2023

Dirbaio Nov 28, 2023

Dirbaio commented Nov 28, 2023 •

edited

bors commented Nov 29, 2023

bors commented Nov 29, 2023

rust-log-analyzer commented Nov 29, 2023

taiki-e commented Nov 29, 2023

Amanieu commented Nov 29, 2023

bors commented Nov 29, 2023

bors commented Nov 29, 2023

rust-log-analyzer commented Nov 29, 2023

MabezDev commented Nov 29, 2023

compiler-errors commented Nov 29, 2023

bors commented Nov 30, 2023

bors commented Nov 30, 2023

rust-timer commented Nov 30, 2023

Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf #114499

Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf #114499

Conversation

taiki-e commented Aug 5, 2023 • edited

taiki-e commented Aug 5, 2023

This comment has been minimized.

MabezDev commented Nov 6, 2023

MabezDev commented Nov 6, 2023

taiki-e commented Nov 7, 2023 • edited

Footnotes

bors commented Nov 8, 2023

MabezDev commented Nov 15, 2023

Dirbaio commented Nov 15, 2023

taiki-e commented Nov 15, 2023

This comment has been minimized.

rustbot commented Nov 15, 2023

Dirbaio commented Nov 15, 2023 • edited

MabezDev commented Nov 28, 2023

Dirbaio Nov 28, 2023

Choose a reason for hiding this comment

Dirbaio commented Nov 28, 2023 • edited

bors commented Nov 29, 2023

bors commented Nov 29, 2023

rust-log-analyzer commented Nov 29, 2023

taiki-e commented Nov 29, 2023

Amanieu commented Nov 29, 2023

bors commented Nov 29, 2023

bors commented Nov 29, 2023

rust-log-analyzer commented Nov 29, 2023

MabezDev commented Nov 29, 2023

compiler-errors commented Nov 29, 2023

bors commented Nov 30, 2023

bors commented Nov 30, 2023

rust-timer commented Nov 30, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

taiki-e commented Aug 5, 2023 •

edited

taiki-e commented Nov 7, 2023 •

edited

Dirbaio commented Nov 15, 2023 •

edited

Dirbaio commented Nov 28, 2023 •

edited