Add integer atomic types #33048

Amanieu · 2016-04-17T11:52:51Z

Tracking issue: #32976
RFC: rust-lang/rfcs#1543

The changes to AtomicBool in the RFC are not included, they are in a separate PR (#32365).

rust-highfive · 2016-04-17T11:52:57Z

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

Amanieu · 2016-04-17T11:53:10Z

r? @alexcrichton

alexcrichton · 2016-04-18T16:26:59Z

Thanks @Amanieu! Some thoughts of mine:

Can you make sure the #[cfg(target_has_atomic)] starts out as unstable as well? The gating here should be similar to things like target_vendor.
Would it be possible to define Atomic{I,U}size as part of the same atomic_int! macro?

alexcrichton · 2016-04-18T16:29:06Z

src/librustc_back/target/aarch64_apple_ios.rs

@@ -23,6 +23,7 @@ pub fn target() -> Target {
        options: TargetOptions {
            features: "+neon,+fp-armv8,+cyclone".to_string(),
            eliminate_frame_pointer: false,
+            max_atomic_width: 128,


Just confirming, but this is inherent to aarch64 (e.g. 128-bit atomics are available everywhere?)

alexcrichton · 2016-04-18T16:31:46Z

Gah, I hit enter too quickly...

Would it be possible to detect cmpxchg16b for on x86_64 for more CPUs? Something like querying LLVM? It seems like that'd be a major use case for this, but it's only supported on OSX right now.
To confirm, platforms like MIPS only have 32-bit atomics but we're saying they have 8-bit atomics, right? Are we really sure that these don't lower to calls in compiler-rt?

I wonder if this can also try to have a more comprehensive test? We don't run tests for platforms like ARM/MIPS/etc, but it'd be good to exercise those platforms as they're quite relevant here.

Also, cc @rust-lang/libs, @brson

Amanieu · 2016-04-18T16:51:49Z

Would it be possible to define Atomic{I,U}size as part of the same atomic_int! macro?

The reason I left them out is because they use different stability attributes (rust1, extended_compare_and_swap and atomic_debug), whereas all the new atomic types are under the integer_atomics feature.

Would it be possible to detect cmpxchg16b for on x86_64 for more CPUs? Something like querying LLVM? It seems like that'd be a major use case for this, but it's only supported on OSX right now.

This is a bit complicated because some early AMD x86_64 CPUs don't support cmpxchg16b. Support for this instruction can be enabled in LLVM by targeting a CPU that supports it (which the OSX target does with core2), or by enabling the cx16 LLVM feature.

I don't think there is a way to extract the maximum atomic size from LLVM's API. I've mostly copied what Clang does, which is to determine it from the target architecture and any compilation flags passed in (-march, -mcx16, etc).

To confirm, platforms like MIPS only have 32-bit atomics but we're saying they have 8-bit atomics, right? Are we really sure that these don't lower to calls in compiler-rt?

Yes, these will never be lowered to compiler-rt calls. The test should be able to catch any cases that are lowered to libcalls.

I wonder if this can also try to have a more comprehensive test? We don't run tests for platforms like ARM/MIPS/etc, but it'd be good to exercise those platforms as they're quite relevant here.

How would you suggest doing this?

Amanieu · 2016-04-18T16:59:26Z

Can you make sure the #[cfg(target_has_atomic)] starts out as unstable as well? The gating here should be similar to things like target_vendor.

Should I reuse the integer_atomics feature gate for this or add a new cfg_target_has_atomic feature gate?

alexcrichton · 2016-04-18T20:58:25Z

The reason I left them out is because they use different stability attributes

Right yeah, but perhaps that could be an input to the macro? (e.g. the macro takes a stability attribute as input)

I've mostly copied what Clang does

Only statically, though, right? If I pass -C target-cpu=native this won't enable cmpxchg16b?

The test should be able to catch any cases that are lowered to libcalls.

Unfortunately this test won't catch anything because it's only run on x86_64 and x86 for now.

How would you suggest doing this?

A "minimal cross-compiled libcore" could probably suffice. Something with a few lang items enough to just generate an object file should be enough. You could then call the intrinsics directly to ensure they're not lowered to libcalls.

Amanieu · 2016-04-19T00:21:10Z

Only statically, though, right? If I pass -C target-cpu=native this won't enable cmpxchg16b?

No, for that we would need to be able to recognize all the CPU variant and set the appropriate LLVM flags for them. See Clang: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20130930/090167.html

Amanieu · 2016-04-19T10:14:35Z

Should we expose compare_and_swap on the new atomic types? The plan is to deprecate it in favor of compare_exchange.

Amanieu · 2016-04-19T10:33:50Z

Also, should I add constants like ATOMIC_U8_INIT for all the new integer types?

Amanieu · 2016-04-19T13:09:53Z

Right yeah, but perhaps that could be an input to the macro? (e.g. the macro takes a stability attribute as input)

Done

A "minimal cross-compiled libcore" could probably suffice. Something with a few lang items enough to just generate an object file should be enough. You could then call the intrinsics directly to ensure they're not lowered to libcalls.

Done

alexcrichton · 2016-04-19T17:49:38Z

No, for that we would need to be able to recognize all the CPU variant and set the appropriate LLVM flags for them. See Clang: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20130930/090167.html

Yeah that's kinda unfortunate for now, although we're not even exposing the 128-bit types just yet, so it's ok to punt on this for now.

Should we expose compare_and_swap on the new atomic types?

Nah I wouldn't bother. If it's easier to write the macro where they all have compare_and_swap (as isize/usize must have it) then that's also fine (no skin off our back).

Also, should I add constants like ATOMIC_U8_INIT for all the new integer types?

Indeed!

Awesome, thanks for the updates!

Amanieu · 2016-04-19T17:58:38Z

I've added the constants, is there anything else to do?

alexcrichton · 2016-04-19T18:04:12Z

Looks good to me, thanks @Amanieu! I want to double check with @rust-lang/libs about the #[cfg(target_has_atomic = "ptr")] for bool/isize/usize/ptr, but I suspect we'll be ok with it for now.

ollie27 · 2016-04-19T19:00:35Z

Are the constants not going to be deprecated once const fn is stabilized?

alexcrichton · 2016-04-19T19:43:26Z

They likely will be, yes, but const fn isn't currently stable so some stable alternative is needed

arielb1 · 2016-05-05T09:17:31Z


../src/libsyntax/feature_gate.rs:272:5: 272:6 error: no rules expected the token `(`
../src/libsyntax/feature_gate.rs:272     (active, cfg_target_has_atomic, "1.10.0", Some(32976)),
                                         ^
make: *** [x86_64-apple-darwin/stage0/lib/rustlib/x86_64-apple-darwin/lib/stamp.syntax] Error 101
make: *** Waiting for unfinished jobs....

missing comma

Amanieu · 2016-05-05T12:55:24Z

Fixed

alexcrichton · 2016-05-05T15:59:12Z

@bors: r+ 1a6530803e1c38a105274d0554306fede183d9ce

bors · 2016-05-06T07:54:07Z

⌛ Testing commit 1a65308 with merge 4a133c2...

bors · 2016-05-06T07:58:58Z

💔 Test failed - auto-linux-64-cross-armsf

Amanieu · 2016-05-06T09:01:24Z

Sorry about that, fixed again.

alexcrichton · 2016-05-06T17:10:18Z

@bors: r+ 033699ca561a7e8d2f33eee392a3a8a7373e891b

bors · 2016-05-06T21:12:18Z

⌛ Testing commit 033699c with merge 780897b...

bors · 2016-05-06T22:13:01Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

bors · 2016-05-07T23:50:56Z

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

…arget

Amanieu · 2016-05-09T12:38:45Z

Rebased and changed the test to only run on Linux.

alexcrichton · 2016-05-09T15:09:12Z

@bors: r+ 03dd9b8

bors · 2016-05-09T15:48:59Z

⌛ Testing commit 03dd9b8 with merge af0a433...

Add integer atomic types Tracking issue: #32976 RFC: rust-lang/rfcs#1543 The changes to AtomicBool in the RFC are not included, they are in a separate PR (#32365).

bors · 2016-05-09T17:57:32Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@alexcrichton

…-abi, r=alexcrichton Update aarch64-linux-android target to match android abi. - Changed `target_env` to "gnu" to empty "" for all android targets because it does not matter for android. - The PR #33048 added "max_atomic_width" for arm-android but missed recently added armv7-android. Add it there too. - Added features `+neon,+fp-armv8` because they [must exist on `aarch64` android](http://developer.android.com/ndk/guides/cpu-features.html). - Update libc to include rust-lang/libc#282 so that rust's std lib works on android's aarch64 (the main issue there was incorrect structure alignment on 64-bit arm). r? @alexcrichton

rust-highfive assigned aturon Apr 17, 2016

rust-highfive assigned alexcrichton and unassigned aturon Apr 17, 2016

Amanieu force-pushed the integer_atomics branch from 5d10633 to 59a66d4 Compare April 17, 2016 22:16

alexcrichton reviewed Apr 18, 2016
View reviewed changes

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 18, 2016

Amanieu force-pushed the integer_atomics branch from 59a66d4 to 7078819 Compare April 18, 2016 16:53

Amanieu force-pushed the integer_atomics branch 3 times, most recently from de5dcb7 to f794e48 Compare April 18, 2016 18:26

Amanieu force-pushed the integer_atomics branch 2 times, most recently from a86ad7f to 702813d Compare April 19, 2016 13:05

Amanieu force-pushed the integer_atomics branch from 702813d to 7a2c77e Compare April 19, 2016 17:58

Amanieu force-pushed the integer_atomics branch from 60bf93f to 1a65308 Compare May 5, 2016 12:46

Amanieu force-pushed the integer_atomics branch from 1a65308 to 033699c Compare May 6, 2016 09:01

Amanieu added 3 commits May 9, 2016 13:31

Add #[cfg(target_has_atomic)] to get atomic support for the current t…

04835ea

…arget

Add new atomic integer types

97216a6

Add test to ensure that atomic types are lock-free

03dd9b8

Amanieu force-pushed the integer_atomics branch from 033699c to 03dd9b8 Compare May 9, 2016 12:33

bors merged commit 03dd9b8 into rust-lang:master May 9, 2016

bors mentioned this pull request May 9, 2016

Update aarch64-linux-android target to match android abi. #33500

Merged

Amanieu mentioned this pull request May 11, 2016

Generic atomic v2 rust-lang/rfcs#1477

Closed

overvenus mentioned this pull request Sep 15, 2016

use Atomic value to improve performance tikv/rust-prometheus#59

Closed

est31 mentioned this pull request Sep 20, 2020

Why does only x86_64_apple_darwin have max_atomic_width set to 128? #76952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integer atomic types #33048

Add integer atomic types #33048

Amanieu commented Apr 17, 2016

rust-highfive commented Apr 17, 2016

Amanieu commented Apr 17, 2016

alexcrichton commented Apr 18, 2016

alexcrichton Apr 18, 2016

Amanieu Apr 18, 2016

alexcrichton commented Apr 18, 2016

Amanieu commented Apr 18, 2016

Amanieu commented Apr 18, 2016

alexcrichton commented Apr 18, 2016

Amanieu commented Apr 19, 2016

Amanieu commented Apr 19, 2016

Amanieu commented Apr 19, 2016

Amanieu commented Apr 19, 2016

alexcrichton commented Apr 19, 2016

Amanieu commented Apr 19, 2016

alexcrichton commented Apr 19, 2016

ollie27 commented Apr 19, 2016

alexcrichton commented Apr 19, 2016

arielb1 commented May 5, 2016 •

edited

Loading

Amanieu commented May 5, 2016

alexcrichton commented May 5, 2016

bors commented May 6, 2016

bors commented May 6, 2016

Amanieu commented May 6, 2016

alexcrichton commented May 6, 2016

bors commented May 6, 2016

bors commented May 6, 2016

bors commented May 7, 2016

Amanieu commented May 9, 2016

alexcrichton commented May 9, 2016

bors commented May 9, 2016

bors commented May 9, 2016

Add integer atomic types #33048

Add integer atomic types #33048

Conversation

Amanieu commented Apr 17, 2016

rust-highfive commented Apr 17, 2016

Amanieu commented Apr 17, 2016

alexcrichton commented Apr 18, 2016

alexcrichton Apr 18, 2016

Choose a reason for hiding this comment

Amanieu Apr 18, 2016

Choose a reason for hiding this comment

alexcrichton commented Apr 18, 2016

Amanieu commented Apr 18, 2016

Amanieu commented Apr 18, 2016

alexcrichton commented Apr 18, 2016

Amanieu commented Apr 19, 2016

Amanieu commented Apr 19, 2016

Amanieu commented Apr 19, 2016

Amanieu commented Apr 19, 2016

alexcrichton commented Apr 19, 2016

Amanieu commented Apr 19, 2016

alexcrichton commented Apr 19, 2016

ollie27 commented Apr 19, 2016

alexcrichton commented Apr 19, 2016

arielb1 commented May 5, 2016 • edited Loading

Amanieu commented May 5, 2016

alexcrichton commented May 5, 2016

bors commented May 6, 2016

bors commented May 6, 2016

Amanieu commented May 6, 2016

alexcrichton commented May 6, 2016

bors commented May 6, 2016

bors commented May 6, 2016

bors commented May 7, 2016

Amanieu commented May 9, 2016

alexcrichton commented May 9, 2016

bors commented May 9, 2016

bors commented May 9, 2016

arielb1 commented May 5, 2016 •

edited

Loading