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

atomic::Ordering "same as LLVM's"? Seems wrong. #65282

Closed
RalfJung opened this issue Oct 10, 2019 · 10 comments · Fixed by #65339
Closed

atomic::Ordering "same as LLVM's"? Seems wrong. #65282

RalfJung opened this issue Oct 10, 2019 · 10 comments · Fixed by #65339
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 10, 2019

@jonas-schievink pointed out that our atomic::Ordering docs say

Rust's memory orderings are the same as LLVM's.

That's not really correct though... at least according to the Nomicon, we use the C11 memory model, which is different from LLVM's. The kinds of ordering that exist are the same (at least insofar as Rust exposes them), but the details of the rules around data races are different.

Code that mixes atomic and non-atomic accesses on the same location can behave differently under both models (concretely, some programs that are UB under C11 are no longer UB under LLVM). So if people interpret "same as LLVM's" too strongly, they might be mislead.

Cc @Gankra @Centril

@jonas-schievink jonas-schievink added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 10, 2019
@HadrienG2
Copy link

HadrienG2 commented Oct 10, 2019

I think this question is actually trickier than it sounds, because I see periodical interest in bringing in some of LLVM's extensions with respect to C11 (unordered, element-wise atomic memcpy...) to Rust.

A clear "we use the C11 memory model, not LLVM's" stance could be interpreted as closing that door.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 10, 2019

Well as far as I am concerned that is the current stance. But it doesn't close any doors. Switching from C11 to LLVM's model makes strictly more programs defined, that's the only difference. IOW, that's a non-breaking change.

Moving from LLVM's to C's model, on the other hand, turns some well-defined programs into UB. So, C11 is also the conservative future-compatible choice here.

@HadrienG2
Copy link

HadrienG2 commented Oct 10, 2019

Moving from LLVM's to C's model, on the other hand, turns some well-defined programs into UB. So, C11 is also the conservative future-compatible choice here.

This is not true of all of LLVM's features though. For example, LLVM's unordered atomics can be emulated with relaxed C11 atomics without loss of correctness (but potentially at a performance cost).

In this respect, there is room for a "we mostly follow the C11 model, but allow ourselves to adopt LLVM extensions which can be pessimistically implemented on top of the C11 building blocks" stance.

@RalfJung
Copy link
Member Author

unordered is not a thing in Rust right now though. :) So for the APIs we currently expose, saying we use C11 is the conservative future-compatible choice -- and I actually think this is the current normative choice, based on what the Nomicon says (it's also what I told everyone over the last years).

Extending Rust's API beyond C11's would change that game -- I'll comment on the "unordered" thread when I find the time. But I don't see any reason why that would be in conflict with saying that right now, Rust follows C11.

@HadrienG2
Copy link

Point taken ;)

@RalfJung
Copy link
Member Author

Oh and I don't think element-wise atomic memcpy is even an extension of the model; that's just an intrinsic for a loop that can already be expressed in the existing model.

@HadrienG2
Copy link

HadrienG2 commented Oct 10, 2019

Well, today LLVM is clearly unable to optimize a loop of the corresponding relaxed atomic load/store operation into a performant implementation of its element-wise memcpy intrinsic, because it is unable to coalesce neighboring atomic loads and stores. But that's arguably an LLVM optimizer issue, not a semantic limitation of C11 atomics.

@RalfJung
Copy link
Member Author

But that's arguably an LLVM optimizer issue, not a semantic limitation of C11 atomics.

Indeed that would have been my argument. :)

@Centril
Copy link
Contributor

Centril commented Oct 11, 2019

Cc @Centril

Seems like the standard library should be fixed. Want to file a PR?

@RalfJung
Copy link
Member Author

PR filed: #65339

@bors bors closed this as completed in 293d02d Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants