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

warn against using intrinsics that leave the scope of our memory model #118128

Merged
merged 1 commit into from
Dec 3, 2023

Conversation

RalfJung
Copy link
Member

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented Nov 21, 2023

r? @joshtriplett

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 21, 2023
@Urgau
Copy link
Member

Urgau commented Nov 21, 2023

Can't we just remove them? At least for atomic_{load,store}_unordered the PR introducing them #19835 didn't have a use-case for them and they don't seems to be used anywhere in this repo (including stdarch).

nontemporal_store is used within stdarch, so that a no go for now

@@ -341,6 +341,7 @@ extern "rust-intrinsic" {
/// [`Ordering::Relaxed`] as the `order`. For example, [`AtomicBool::load`].
#[rustc_nounwind]
pub fn atomic_load_relaxed<T: Copy>(src: *const T) -> T;
/// Do NOT use this intrinsic; "unordered" operations do not exist in our memory model!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC they were added for experimentation. Maybe the warning could be a bit more informative?

For whom is that warning anyway? std/compiler devs or for nightly users?
I.e. are you trying to convey that they shouldn't be exposed through stable std APIs or that nightly users are moving on thin ice when they experiment with this? Because the latter could be reasonable if they're doing that to experiment and demonstrate performance differences.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have clarified: in terms of the AM, I think the best that we can do is that these intrinsics act like regular non-atomic reads and writes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrrm, I still think the language is a bit too unspecific. Then again, people who are just doing experiments are free to ignore any such warning anyway, so whatever.

library/core/src/intrinsics.rs Show resolved Hide resolved
@RalfJung
Copy link
Member Author

Can't we just remove them? At least for atomic_{load,store}_unordered the PR introducing them #19835 didn't have a use-case for them and they don't seems to be used anywhere in this repo (including stdarch).

They are used by compiler_builtins. We'll need to take a close look at that eventually, but for now we can at least discourage further usage of them.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 2, 2023

@the8472 can you take the review of this one, since you already took a look?

@the8472 the8472 assigned the8472 and unassigned joshtriplett Dec 2, 2023
@the8472
Copy link
Member

the8472 commented Dec 2, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 2, 2023

📌 Commit 79ad512 has been approved by the8472

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 2, 2023
@bors
Copy link
Contributor

bors commented Dec 3, 2023

⌛ Testing commit 79ad512 with merge 2f1ba4a...

@bors
Copy link
Contributor

bors commented Dec 3, 2023

☀️ Test successful - checks-actions
Approved by: the8472
Pushing 2f1ba4a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 3, 2023
@bors bors merged commit 2f1ba4a into rust-lang:master Dec 3, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 3, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2f1ba4a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.5% [2.5%, 2.5%] 1
Improvements ✅
(primary)
-0.1% [-0.1%, -0.1%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.1% [-0.1%, -0.1%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.67s -> 671.206s (-0.22%)
Artifact size: 314.11 MiB -> 314.12 MiB (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants