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

Simplify memory ordering intrinsics #97423

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented May 26, 2022

This changes the names of the atomic intrinsics to always fully include their memory ordering arguments.

- atomic_cxchg
+ atomic_cxchg_seqcst_seqcst

- atomic_cxchg_acqrel
+ atomic_cxchg_acqrel_release

- atomic_cxchg_acqrel_failrelaxed
+ atomic_cxchg_acqrel_relaxed

// And so on.
  • seqcst is no longer implied
  • The failure ordering on chxchg is no longer implied in some cases, but now always explicitly part of the name.
  • release is no longer shortened to just rel. That was especially confusing, since relaxed also starts with rel.
  • acquire is no longer shortened to just acq, such that the names now all match the std::sync::atomic::Ordering variants exactly.
  • This now allows for more combinations on the compare exchange operations, such as atomic_cxchg_acquire_release, which is necessary for Lift unnecessary restriction on CAS failure ordering #68464.
  • This PR only exposes the new possibilities through unstable intrinsics, but not yet through the stable API. That's for a separate PR that requires an FCP.

Suffixes for operations with a single memory order:

Order Before After
Relaxed _relaxed _relaxed
Acquire _acq _acquire
Release _rel _release
AcqRel _acqrel _acqrel
SeqCst (none) _seqcst

Suffixes for compare-and-exchange operations with two memory orderings:

Success Failure Before After
Relaxed Relaxed _relaxed _relaxed_relaxed
Relaxed Acquire _relaxed_acquire
Relaxed SeqCst _relaxed_seqcst
Acquire Relaxed _acq_failrelaxed _acquire_relaxed
Acquire Acquire _acq _acquire_acquire
Acquire SeqCst _acquire_seqcst
Release Relaxed _rel _release_relaxed
Release Acquire _release_acquire
Release SeqCst _release_seqcst
AcqRel Relaxed _acqrel_failrelaxed _acqrel_relaxed
AcqRel Acquire _acqrel _acqrel_acquire
AcqRel SeqCst _acqrel_seqcst
SeqCst Relaxed _failrelaxed _seqcst_relaxed
SeqCst Acquire _failacq _seqcst_acquire
SeqCst SeqCst (none) _seqcst_seqcst

@m-ou-se m-ou-se added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-intrinsics Area: Intrinsics labels May 26, 2022
@rust-highfive

This comment was marked as off-topic.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 26, 2022
@m-ou-se m-ou-se force-pushed the memory-ordering-intrinsics branch from 1ae453e to 123c92a Compare May 26, 2022 10:47
@m-ou-se
Copy link
Member Author

m-ou-se commented May 26, 2022

cc @tmiasko

@m-ou-se
Copy link
Member Author

m-ou-se commented May 26, 2022

This updates the stdarch submodule to include rust-lang/stdarch@dc4b2e2, which isn't merged in stdarch's main branch yet, since that change needs to happen at the same time as the rest of this PR.

@rust-log-analyzer

This comment has been minimized.

@tmiasko
Copy link
Contributor

tmiasko commented May 26, 2022

Looks good to me. Thanks!

r=me with commits squashed.

@rust-log-analyzer

This comment has been minimized.

@m-ou-se m-ou-se force-pushed the memory-ordering-intrinsics branch from 7d48e22 to 512099e Compare May 26, 2022 12:18
@m-ou-se
Copy link
Member Author

m-ou-se commented May 26, 2022

This updates the stdarch submodule to include rust-lang/stdarch@dc4b2e2, which isn't merged in stdarch's main branch yet, since that change needs to happen at the same time as the rest of this PR.

cc @Amanieu for stdarch ^

@m-ou-se m-ou-se force-pushed the memory-ordering-intrinsics branch from 512099e to 126a41f Compare May 26, 2022 14:30
@tmiasko
Copy link
Contributor

tmiasko commented May 27, 2022

This updates the stdarch submodule to include rust-lang/stdarch@dc4b2e2, which isn't merged in stdarch's main branch yet, since that change needs to happen at the same time as the rest of this PR.

Since the core_arch uses intrinsics declared in the core, for the not(bootstrap) case we can also add re-exports under old name for those handful of intrinsics that are still required, and update stdarch using the usual process at some later point.

@m-ou-se m-ou-se force-pushed the memory-ordering-intrinsics branch from 126a41f to 2d02416 Compare June 22, 2022 11:09
@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 22, 2022

Oops, got distracted.

I've updated the PR following your suggestion. :)

@tmiasko
Copy link
Contributor

tmiasko commented Jun 22, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jun 22, 2022

📌 Commit 2d02416 has been approved by tmiasko

@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 Jun 22, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 26, 2022
… r=tmiasko

Simplify memory ordering intrinsics

This changes the names of the atomic intrinsics to always fully include their memory ordering arguments.

```diff
- atomic_cxchg
+ atomic_cxchg_seqcst_seqcst

- atomic_cxchg_acqrel
+ atomic_cxchg_acqrel_release

- atomic_cxchg_acqrel_failrelaxed
+ atomic_cxchg_acqrel_relaxed

// And so on.
```

- `seqcst` is no longer implied
- The failure ordering on chxchg is no longer implied in some cases, but now always explicitly part of the name.
- `release` is no longer shortened to just `rel`. That was especially confusing, since `relaxed` also starts with `rel`.
- `acquire` is no longer shortened to just `acq`, such that the names now all match the `std::sync::atomic::Ordering` variants exactly.
- This now allows for more combinations on the compare exchange operations, such as `atomic_cxchg_acquire_release`, which is necessary for rust-lang#68464.
- This PR only exposes the new possibilities through unstable intrinsics, but not yet through the stable API. That's for [a separate PR](rust-lang#98383) that requires an FCP.

Suffixes for operations with a single memory order:

| Order   | Before       | After      |
|---------|--------------|------------|
| Relaxed | `_relaxed`   | `_relaxed` |
| Acquire | `_acq`       | `_acquire` |
| Release | `_rel`       | `_release` |
| AcqRel  | `_acqrel`    | `_acqrel`  |
| SeqCst  | (none)       | `_seqcst`  |

Suffixes for compare-and-exchange operations with two memory orderings:

| Success | Failure | Before                   | After              |
|---------|---------|--------------------------|--------------------|
| Relaxed | Relaxed | `_relaxed`               | `_relaxed_relaxed` |
| Relaxed | Acquire | ❌                      | `_relaxed_acquire` |
| Relaxed | SeqCst  | ❌                      | `_relaxed_seqcst`  |
| Acquire | Relaxed | `_acq_failrelaxed`       | `_acquire_relaxed` |
| Acquire | Acquire | `_acq`                   | `_acquire_acquire` |
| Acquire | SeqCst  | ❌                      | `_acquire_seqcst`  |
| Release | Relaxed | `_rel`                   | `_release_relaxed` |
| Release | Acquire | ❌                      | `_release_acquire` |
| Release | SeqCst  | ❌                      | `_release_seqcst`  |
| AcqRel  | Relaxed | `_acqrel_failrelaxed`    | `_acqrel_relaxed`  |
| AcqRel  | Acquire | `_acqrel`                | `_acqrel_acquire`  |
| AcqRel  | SeqCst  | ❌                      | `_acqrel_seqcst`   |
| SeqCst  | Relaxed | `_failrelaxed`           | `_seqcst_relaxed`  |
| SeqCst  | Acquire | `_failacq`               | `_seqcst_acquire`  |
| SeqCst  | SeqCst  | (none)                   | `_seqcst_seqcst`   |
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 26, 2022
… r=tmiasko

Simplify memory ordering intrinsics

This changes the names of the atomic intrinsics to always fully include their memory ordering arguments.

```diff
- atomic_cxchg
+ atomic_cxchg_seqcst_seqcst

- atomic_cxchg_acqrel
+ atomic_cxchg_acqrel_release

- atomic_cxchg_acqrel_failrelaxed
+ atomic_cxchg_acqrel_relaxed

// And so on.
```

- `seqcst` is no longer implied
- The failure ordering on chxchg is no longer implied in some cases, but now always explicitly part of the name.
- `release` is no longer shortened to just `rel`. That was especially confusing, since `relaxed` also starts with `rel`.
- `acquire` is no longer shortened to just `acq`, such that the names now all match the `std::sync::atomic::Ordering` variants exactly.
- This now allows for more combinations on the compare exchange operations, such as `atomic_cxchg_acquire_release`, which is necessary for rust-lang#68464.
- This PR only exposes the new possibilities through unstable intrinsics, but not yet through the stable API. That's for [a separate PR](rust-lang#98383) that requires an FCP.

Suffixes for operations with a single memory order:

| Order   | Before       | After      |
|---------|--------------|------------|
| Relaxed | `_relaxed`   | `_relaxed` |
| Acquire | `_acq`       | `_acquire` |
| Release | `_rel`       | `_release` |
| AcqRel  | `_acqrel`    | `_acqrel`  |
| SeqCst  | (none)       | `_seqcst`  |

Suffixes for compare-and-exchange operations with two memory orderings:

| Success | Failure | Before                   | After              |
|---------|---------|--------------------------|--------------------|
| Relaxed | Relaxed | `_relaxed`               | `_relaxed_relaxed` |
| Relaxed | Acquire | ❌                      | `_relaxed_acquire` |
| Relaxed | SeqCst  | ❌                      | `_relaxed_seqcst`  |
| Acquire | Relaxed | `_acq_failrelaxed`       | `_acquire_relaxed` |
| Acquire | Acquire | `_acq`                   | `_acquire_acquire` |
| Acquire | SeqCst  | ❌                      | `_acquire_seqcst`  |
| Release | Relaxed | `_rel`                   | `_release_relaxed` |
| Release | Acquire | ❌                      | `_release_acquire` |
| Release | SeqCst  | ❌                      | `_release_seqcst`  |
| AcqRel  | Relaxed | `_acqrel_failrelaxed`    | `_acqrel_relaxed`  |
| AcqRel  | Acquire | `_acqrel`                | `_acqrel_acquire`  |
| AcqRel  | SeqCst  | ❌                      | `_acqrel_seqcst`   |
| SeqCst  | Relaxed | `_failrelaxed`           | `_seqcst_relaxed`  |
| SeqCst  | Acquire | `_failacq`               | `_seqcst_acquire`  |
| SeqCst  | SeqCst  | (none)                   | `_seqcst_seqcst`   |
@m-ou-se m-ou-se force-pushed the memory-ordering-intrinsics branch from 510af15 to 4982a59 Compare June 28, 2022 06:58
@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 28, 2022

@bors r=tmiasko

@bors
Copy link
Contributor

bors commented Jun 28, 2022

📌 Commit 4982a59 has been approved by tmiasko

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 28, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 28, 2022
… r=tmiasko

Simplify memory ordering intrinsics

This changes the names of the atomic intrinsics to always fully include their memory ordering arguments.

```diff
- atomic_cxchg
+ atomic_cxchg_seqcst_seqcst

- atomic_cxchg_acqrel
+ atomic_cxchg_acqrel_release

- atomic_cxchg_acqrel_failrelaxed
+ atomic_cxchg_acqrel_relaxed

// And so on.
```

- `seqcst` is no longer implied
- The failure ordering on chxchg is no longer implied in some cases, but now always explicitly part of the name.
- `release` is no longer shortened to just `rel`. That was especially confusing, since `relaxed` also starts with `rel`.
- `acquire` is no longer shortened to just `acq`, such that the names now all match the `std::sync::atomic::Ordering` variants exactly.
- This now allows for more combinations on the compare exchange operations, such as `atomic_cxchg_acquire_release`, which is necessary for rust-lang#68464.
- This PR only exposes the new possibilities through unstable intrinsics, but not yet through the stable API. That's for [a separate PR](rust-lang#98383) that requires an FCP.

Suffixes for operations with a single memory order:

| Order   | Before       | After      |
|---------|--------------|------------|
| Relaxed | `_relaxed`   | `_relaxed` |
| Acquire | `_acq`       | `_acquire` |
| Release | `_rel`       | `_release` |
| AcqRel  | `_acqrel`    | `_acqrel`  |
| SeqCst  | (none)       | `_seqcst`  |

Suffixes for compare-and-exchange operations with two memory orderings:

| Success | Failure | Before                   | After              |
|---------|---------|--------------------------|--------------------|
| Relaxed | Relaxed | `_relaxed`               | `_relaxed_relaxed` |
| Relaxed | Acquire | ❌                      | `_relaxed_acquire` |
| Relaxed | SeqCst  | ❌                      | `_relaxed_seqcst`  |
| Acquire | Relaxed | `_acq_failrelaxed`       | `_acquire_relaxed` |
| Acquire | Acquire | `_acq`                   | `_acquire_acquire` |
| Acquire | SeqCst  | ❌                      | `_acquire_seqcst`  |
| Release | Relaxed | `_rel`                   | `_release_relaxed` |
| Release | Acquire | ❌                      | `_release_acquire` |
| Release | SeqCst  | ❌                      | `_release_seqcst`  |
| AcqRel  | Relaxed | `_acqrel_failrelaxed`    | `_acqrel_relaxed`  |
| AcqRel  | Acquire | `_acqrel`                | `_acqrel_acquire`  |
| AcqRel  | SeqCst  | ❌                      | `_acqrel_seqcst`   |
| SeqCst  | Relaxed | `_failrelaxed`           | `_seqcst_relaxed`  |
| SeqCst  | Acquire | `_failacq`               | `_seqcst_acquire`  |
| SeqCst  | SeqCst  | (none)                   | `_seqcst_seqcst`   |
This was referenced Jun 28, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 29, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#97423 (Simplify memory ordering intrinsics)
 - rust-lang#97542 (Use typed indices in argument mismatch algorithm)
 - rust-lang#97786 (Account for `-Z simulate-remapped-rust-src-base` when resolving remapped paths)
 - rust-lang#98277 (Fix trait object reborrow suggestion)
 - rust-lang#98525 (Add regression test for rust-lang#79224)
 - rust-lang#98549 (interpret: do not prune requires_caller_location stack frames quite so early)
 - rust-lang#98603 (Some borrowck diagnostic fixes)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 45740ac into rust-lang:master Jun 29, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jun 29, 2022
@m-ou-se m-ou-se deleted the memory-ordering-intrinsics branch June 29, 2022 12:48
celinval added a commit to celinval/kani-dev that referenced this pull request Jul 6, 2022
The updates required were related to the following changes:
  - Simplify memory ordering intrinsics
    - rust-lang/rust#97423
  - once cell renamings
    - rust-lang/rust#98165
  - Rename the ConstS::val field as kind
    - rust-lang/rust#97935
  - Remove the source archive functionality of ArchiveWriter
    - rust-lang/rust#98098
  - Use valtrees as the type-system representation for constant values
    - rust-lang/rust#96591
celinval added a commit to model-checking/kani that referenced this pull request Jul 6, 2022
* Update toolchain to 2022-07-05

The updates required were related to the following changes:
  - Simplify memory ordering intrinsics
    - rust-lang/rust#97423
  - once cell renamings
    - rust-lang/rust#98165
  - Rename the ConstS::val field as kind
    - rust-lang/rust#97935
  - Remove the source archive functionality of ArchiveWriter
    - rust-lang/rust#98098
  - Use valtrees as the type-system representation for constant values
    - rust-lang/rust#96591

* Codegen unimplemented for unsupported constant slices

See #1339 for more details.

* Fix copyright check

* Use codegen_option_span instead
tmiasko added a commit to tmiasko/rust that referenced this pull request Jul 20, 2022
* Add a test for atomic operations introduced in rust-lang#97423 & rust-lang#98383.
* Add a test for fallback code generation strategy used on LLVM 12
  introduced in rust-lang#98385. Use a separate test case instead of a revision
  system since test will be gone once LLVM 12 is no longer supported.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2022
Test codegen of atomic compare-exchange with additional memory orderings

* Add a test for atomic operations introduced in rust-lang#97423 & rust-lang#98383.
* Add a test for fallback code generation strategy used on LLVM 12 introduced in rust-lang#98385. Use a separate test case instead of a revision system since test will be gone once LLVM 12 is no longer supported.
@kkysen
Copy link

kkysen commented Aug 11, 2022

It seems that the error messages for these updated intrinsics are out-of-date with the new names, as they say similarly named function `{old_instrinsic_name}` defined here while pointing to the definition of the new intrinsic name. For example,

error[E0425]: cannot find function `atomic_xchg_acq` in module `core::intrinsics`
   --> src/atomics.rs:167:33
    |
167 |         ) = ::core::intrinsics::atomic_xchg_acq(&mut x, 33 as libc::c_int);
    |                                 ^^^^^^^^^^^^^^^ help: a function with a similar name exists: `atomic_cxchg_acq`
    |
   ::: /home/kkysen/.rustup/toolchains/nightly-2022-08-08-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/intrinsics.rs:320:5
    |
320 |     pub fn atomic_cxchg_acquire_acquire<T: Copy>(dst: *mut T, old: T, src: T) -> (T, bool);
    |     -------------------------------------------------------------------------------------- similarly named function `atomic_cxchg_acq` defined here

error[E0425]: cannot find function `atomic_store_rel` in module `core::intrinsics`
   --> src/atomics.rs:171:25
    |
171 |     ::core::intrinsics::atomic_store_rel(&mut x, 0);
    |                         ^^^^^^^^^^^^^^^^ help: a function with a similar name exists: `atomic_store`
    |
   ::: /home/kkysen/.rustup/toolchains/nightly-2022-08-08-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/intrinsics.rs:523:5
    |
523 |     pub fn atomic_store_seqcst<T: Copy>(dst: *mut T, val: T);
    |     -------------------------------------------------------- similarly named function `atomic_store` defined here

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

Successfully merging this pull request may close these issues.

9 participants