Skip to content
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

Merged
merged 1 commit into from
Jan 2, 2019

Conversation

alexcrichton
Copy link
Member

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!

@alexcrichton
Copy link
Member Author

Note that this is intended to be coupled with rust-lang/rust#56826

coresimd/x86_64/mod.rs Outdated Show resolved Hide resolved
coresimd/x86_64/mod.rs Outdated Show resolved Hide resolved
coresimd/x86_64/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@gnzlbg gnzlbg left a 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.

@alexcrichton alexcrichton force-pushed the cmpxchg16b branch 2 times, most recently from 1498f93 to 8e65a45 Compare December 21, 2018 15:41
@alexcrichton
Copy link
Member Author

Thanks for taking a look! I think I've taken care of everything now.

FWIW I also found the cx16 feature name, but I couldn't actually find any information from Intel saying that it was the official name of this feature. All I could find with Intel was "cmpxchg16", so that's what I ended up naming it in the compiler itself.

Copy link
Contributor

@gnzlbg gnzlbg left a 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...).

/// # Panics
///
/// The `success` ordering must be stronger or equal to `failure`, or this
/// function will panic or abort. See the `Atomic*` documentation's
Copy link
Contributor

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!
@alexcrichton
Copy link
Member Author

Good idea! Updated now

@gnzlbg gnzlbg merged commit b7b1a3f into rust-lang:master Jan 2, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 2, 2019

Thanks!

@the8472
Copy link
Member

the8472 commented Jan 5, 2019

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!

Wouldn't it be sufficient to make it #[cfg(target_feature = "cmpxchg16b")] AtomicU128?

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 5, 2019 via email

@the8472
Copy link
Member

the8472 commented Jan 5, 2019

Yeah, but it's a simple change that's better than not providing it at all.

@alexcrichton alexcrichton deleted the cmpxchg16b branch January 7, 2019 15:28
@alexcrichton
Copy link
Member Author

@the8472 the purpose of this crate is to provide the fundamentals rather than full-blown abstractions, it's possible to create an AtomicU128 crate on crates.io with this support.

The platform support of AtomicU128 means that it's unlikely to be stabilized as-is in libstd any time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants