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

Generic atomic v2 #1477

Closed
wants to merge 1 commit into from
Closed

Generic atomic v2 #1477

wants to merge 1 commit into from

Conversation

mahkoh
Copy link
Contributor

@mahkoh mahkoh commented Jan 25, 2016

Add compiler support for generic atomic operations.

@nrc nrc added T-lang Relevant to the language team, which will review and decide on the RFC. T-compiler Relevant to the compiler team, which will review and decide on the RFC. labels Jan 25, 2016
@Amanieu
Copy link
Member

Amanieu commented Jan 26, 2016

Here is some discussion in internals about possible designs for extended atomic types: https://internals.rust-lang.org/t/pre-rfc-extended-atomic-types/3068

One thing that is really missing from all Atomic<T> proposals is the ability to pad and align T correctly so that it can fit in an atomic integer type. For example, Atomic<(u8, u8, u8)> would ideally have a size and alignment of 4 which would map it to an atomic i32. However this is not possible with the language as it is.

@mahkoh
Copy link
Contributor Author

mahkoh commented Jan 26, 2016

This RFC has little to do with the design of an Atomic<T> type. Given #1478, it's only about the introduction of a single intrinsic.

@Amanieu
Copy link
Member

Amanieu commented Apr 9, 2016

I don't think this RFC is necessary anymore. See atomic-rs for an example of supporting a generic Atomic<T> without any additional compiler support.

@nikomatsakis nikomatsakis added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed I-nominated labels May 5, 2016
@nikomatsakis
Copy link
Contributor

Hear ye, hear ye! This RFC is now entering final comment period.

@rkjnsn
Copy link
Contributor

rkjnsn commented May 10, 2016

In general, it seems useful to be able to query whether atomic operations for a certain size/type are available.

@Amanieu
Copy link
Member

Amanieu commented May 11, 2016

This was recently added by rust-lang/rust#33048 (should be in the latest nightly). See

@nikomatsakis
Copy link
Contributor

This RFC feels like it is superceded in two respects: one, by rust-lang/rust#33048, and secondly, by #1478, which offers similar capabilities. So I'm currently inclined not to accept.

@mahkoh
Copy link
Contributor Author

mahkoh commented May 12, 2016

For a given type T, how would one check if the target supports atomic operations on it?

@Amanieu
Copy link
Member

Amanieu commented May 12, 2016

@mahkoh Like this: https://github.com/Amanieu/atomic-rs/blob/master/src/nightly.rs#L39

(note that atomic-rs hasn't been updated to use the #[cfg(target_has_atomic)] options yet)

@mahkoh
Copy link
Contributor Author

mahkoh commented May 12, 2016

        #[cfg(any(target_pointer_width = "64", target_arch = "x86", target_arch = "arm"))]

So you need target specific knowledge and the code has to be updated whenever a target is added. Furthermore, this doesn't handle the case where the architecture allows atomic operations with the alignment not a multiple of the size.

@Amanieu
Copy link
Member

Amanieu commented May 12, 2016

@mahkoh I've just updated the code, maybe this should make it clearer: https://github.com/Amanieu/atomic-rs/blob/master/src/nightly.rs#L42

Furthermore, this doesn't handle the case where the architecture allows atomic operations with the alignment not a multiple of the size.

No architecture currently supports that and LLVM certainly doesn't since it only supports atomic operations on 8, 16, 32, 64 and 128-bit integer types.

@mahkoh
Copy link
Contributor Author

mahkoh commented May 12, 2016

https://github.com/Amanieu/atomic-rs/blob/master/src/nightly.rs#L42

This seems already outdated because it doesn't handle 128 bit types which you claim LLVM handles. In any case, I don't want to know what the architecture supports or what LLVM supports. I don't want to do my own research. The compiler has already done it for me. So I want the compiler to tell me.

@Amanieu
Copy link
Member

Amanieu commented May 12, 2016

128-bit atomic support is blocked on RFC #1504. And I don't think any architecture will ever support unaligned atomics since that involves atomic operations crossing cache line boundaries.

@mahkoh
Copy link
Contributor Author

mahkoh commented May 12, 2016

How is it blocked on that RFC? Just because there are no 128 bit integers doesn't mean that there aren't any 128 bit aligned 128 types.

@Amanieu
Copy link
Member

Amanieu commented May 12, 2016

LLVM atomic instructions only accept integer types, and the Rust atomic intrinsics were changed to also only accept integer types. This means that you need a 128-bit integer to perform a 128-bit atomic operation.

I guess you could add a hack in the atomic intrinsics to support 128-bit atomics with a non-integer type, but I feel it's not worth the effort (not much demand for 128-bit atomics) so we might as well wait for full 128-bit integer support.

@mahkoh
Copy link
Contributor Author

mahkoh commented May 12, 2016

I see that the situation is worse than I initially assumed. The existence of atomic intrinsics for arbitrary types is of course necessary for this RFC to make any sense. So the RFC has to be amended with the requirement that the intrinsics translate arbitrary types to fitting integer types themselves.

@nikomatsakis
Copy link
Contributor

We discussed in the @rust-lang/lang meeting and we have decided not to accept this RFC. In particular, the @rust-lang/libs team also recently visited the question of atomics in the form of #1543, and decided to adopt the approach advocated there, which seems to somewhat obviate the need for this approach. Thanks @mahkoh!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants