-
Notifications
You must be signed in to change notification settings - Fork 276
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
Add a x86_64::cmpxchg16b
intrinsic
#624
Conversation
Note that this is intended to be coupled with rust-lang/rust#56826 |
41a9035
to
3ae8aa7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The request to rename the feature to cx16
, can be done in a different PR after this is merged (that would require updating rustc). The other comments are mostly nits. I don't know if this is worth putting into its own module, but there is also a cx8
feature so maybe a cx
module might make sense.
1498f93
to
8e65a45
Compare
Thanks for taking a look! I think I've taken care of everything now. FWIW I also found the |
8e65a45
to
4abf44d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one nitpick left: I think that saying that this panics or aborts is incorrect. We can either state that the behavior is undefined, if we want to change the behavior to a panic or abort later on, or we can state what actually happens here (raises an invalid opcode exception...).
coresimd/x86_64/cmpxchg16b.rs
Outdated
/// # Panics | ||
/// | ||
/// The `success` ordering must be stronger or equal to `failure`, or this | ||
/// function will panic or abort. See the `Atomic*` documentation's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function neither panics nor aborts, it: "raises an invalid opcode exception in all operating modes".
This intrinsic isn't actually specified by Intel, but it's something gated with CPUID and can otherwise be a useful thing to have when building primitives! There exists an `AtomicU128` type in the standard library but it's only exposed currently (and it's unstable) when a platform fully supports 128-bit atomics. The x86_64 architecture does not support it *unless* the `cmpxchg16b` instruction is available, and it isn't always available! This commit is also a proposal for how we can include support for 128-bit atomics in the standard library on relevant platforms. I'm thinking that we'll expose this one low-level intrinsic in `std::arch::x86_64`, and then if desired a crate on crates.io can build `AtomicU128` from this API. In any case this is all unstable regardless!
4abf44d
to
98c489f
Compare
Good idea! Updated now |
Thanks! |
Wouldn't it be sufficient to make it |
I don’t think that allows using run time feature detection to , E.g., use
atomic if available or a lock otherwise.
|
Yeah, but it's a simple change that's better than not providing it at all. |
@the8472 the purpose of this crate is to provide the fundamentals rather than full-blown abstractions, it's possible to create an The platform support of |
This intrinsic isn't actually specified by Intel, but it's something
gated with CPUID and can otherwise be a useful thing to have when
building primitives!
There exists an
AtomicU128
type in the standard library but it's onlyexposed currently (and it's unstable) when a platform fully supports
128-bit atomics. The x86_64 architecture does not support it unless
the
cmpxchg16b
instruction is available, and it isn't always available!This commit is also a proposal for how we can include support for
128-bit atomics in the standard library on relevant platforms. I'm
thinking that we'll expose this one low-level intrinsic in
std::arch::x86_64
, and then if desired a crate on crates.io can buildAtomicU128
from this API.In any case this is all unstable regardless!