Skip to content

atomic_load intrinsic: use const generic parameter for ordering #141507

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RalfJung
Copy link
Member

We have a gazillion intrinsics for the atomics because we encode the ordering into the intrinsic name rather than making it a parameter. This is particularly bad for those operations that take two orderings. Let's fix that!

This PR only converts load, to see if there's any feedback that would fundamentally change the strategy we pursue for the const generic intrinsics.

The first two commits are preparation and could be a separate PR if you prefer.

@BoxyUwU -- I hope this is a use of const generics that is unlikely to explode? All we need is a const generic of enum type. We could funnel it through an integer if we had to but an enum is obviously nicer...

@bjorn3 it seems like the cranelift backend entirely ignores the ordering?

@rustbot
Copy link
Collaborator

rustbot commented May 24, 2025

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 24, 2025

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@RalfJung RalfJung force-pushed the atomic-intrinsics branch from 23b2539 to 4c7129b Compare May 24, 2025 14:46
@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented May 24, 2025

@bjorn3 it seems like the cranelift backend entirely ignores the ordering?

Correct. Wasm and by extension Cranelift only have SeqCst atomics.

let discr = ord.valtree.unwrap_branch()[0].unwrap_leaf();
let ord = discr.to_atomic_ordering();
// We have to translate from the intrinsic ordering to the backend ordering.
use std::intrinsics::AtomicOrdering;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably just embed the discriminants directly in cg_ssa. The libstd cg_ssa is compiled against may use different discriminants from the one that rustc will end up compiling.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed what we already do for cmp::Ordering where we also rely on the discriminants to be the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering is stable with guaranteed discriminant values, AtomicOrdering is not stable and I assume never will be, or at least not in the current location.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AtomicOrdering got specifically added for this very purpose. It's going to be exactly as stable as we need it for this purpose.

@RalfJung RalfJung force-pushed the atomic-intrinsics branch 2 times, most recently from 8dbc7a6 to 0cd5bdb Compare May 24, 2025 15:46
@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the atomic-intrinsics branch from 0cd5bdb to dc1bd79 Compare May 24, 2025 16:43
@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the atomic-intrinsics branch from dc1bd79 to a9e3996 Compare May 24, 2025 17:11
@BoxyUwU
Copy link
Member

BoxyUwU commented May 24, 2025

I hope this is a use of const generics that is unlikely to explode

I think that anything the compiler lets you do with solely adt_const_params (i.e. not unsized_const_params) should work perfectly fine. AFAIK the only outstanding issue with the feature is the problems around CTFE being invoked on illformed bodies but that should only matter for code that doesn't compile. tl;dr I don't see any problems with using an enum in const generics in std.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants