Skip to content

Move select_unpredictable to the hint module #139726

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

Merged
merged 1 commit into from
Apr 13, 2025

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Apr 12, 2025

There has been considerable discussion in both the ACP (rust-lang/libs-team#468) and tracking issue (#133962) about whether the bool::select_unpredictable method should be in core::hint instead.

I believe this is the right move for the following reasons:

  • The documentation explicitly says that it is a hint, not a codegen guarantee.
  • bool doesn't have a corresponding select method, and I don't think we should be adding one.
  • This shouldn't be something that people reach for with auto-completion unless they specifically understand the interactions with branch prediction. Using conditional moves can easily make code slower by preventing the CPU from speculating past the condition due to the data dependency.
  • Although currently core::hint only contains no-ops, this isn't a hard rule (for example unreachable_unchecked is a bit of a gray area). The documentation only status that the module contains "hints to compiler that affects how code should be emitted or optimized". This is consistent with what select_unpredictable does.

@rustbot
Copy link
Collaborator

rustbot commented Apr 12, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 12, 2025

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 12, 2025
@rust-log-analyzer

This comment has been minimized.

@Amanieu Amanieu force-pushed the select_unpredictable_hint branch from 3645abb to 5d90ccb Compare April 13, 2025 00:34
@Amanieu Amanieu added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 13, 2025
@Amanieu
Copy link
Member Author

Amanieu commented Apr 13, 2025

r? libs-api

@rustbot rustbot assigned dtolnay and unassigned ibraheemdev Apr 13, 2025
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Looks good. I agree with the rationale for the move, and also ready to stabilize, like you have suggested in #133962 (comment).

I wondered whether we are consistent about whether to pub use intrinsics in their stable location, or whether to wrap like in this PR, but we seem to do it both ways.

#[stable(feature = "rust1", since = "1.0.0")]
#[doc(inline)]
pub use crate::intrinsics::transmute;

#[stable(feature = "rust1", since = "1.0.0")]
#[doc(inline)]
pub use crate::intrinsics::copy;
#[stable(feature = "rust1", since = "1.0.0")]
#[doc(inline)]
pub use crate::intrinsics::copy_nonoverlapping;
#[stable(feature = "rust1", since = "1.0.0")]
#[doc(inline)]
pub use crate::intrinsics::write_bytes;

#[inline]
#[stable(feature = "bench_black_box", since = "1.66.0")]
#[rustc_const_stable(feature = "const_black_box", since = "1.86.0")]
pub const fn black_box<T>(dummy: T) -> T {
crate::intrinsics::black_box(dummy)
}

@dtolnay
Copy link
Member

dtolnay commented Apr 13, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 13, 2025

📌 Commit 5d90ccb has been approved by dtolnay

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 Apr 13, 2025
@RalfJung
Copy link
Member

I wondered whether we are consistent about whether to pub use intrinsics in their stable location, or whether to wrap like in this PR, but we seem to do it both ways.

There's a difference between the two: pub used intrinsics cannot be turned into function pointers. AFAIK the only intrinsic we pub use these days is transmute since there it is required for the size check.

Note that intrinsics::copy etc that you cite are not the intrinsics, they are functions wrapping the intrinsic. Making them direct intrinsic re-exports again would be a breaking change due to the fn ptr issue, and making only the items in core::intrinsics actually be intrinsics would still be a breaking change as those paths are accidentally stable since the compiler used to not enforce stability on every path component.

/// search.
///
/// Note however that this lowering is not guaranteed (on any platform) and
/// should not be relied upon when trying to write constant-time code. Also
Copy link
Member

Choose a reason for hiding this comment

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

This should probably say "cryptographic constant-time"; the term "constant-time" also often refers to algorithms that run in O(1). Sadly two CS sub-communities picked the same term for different concepts and both are very widely used.

ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request Apr 13, 2025
… r=dtolnay

Move `select_unpredictable` to the `hint` module

There has been considerable discussion in both the ACP (rust-lang/libs-team#468) and tracking issue (rust-lang#133962) about whether the `bool::select_unpredictable` method should be in `core::hint` instead.

I believe this is the right move for the following reasons:
- The documentation explicitly says that it is a hint, not a codegen guarantee.
- `bool` doesn't have a corresponding `select` method, and I don't think we should be adding one.
- This shouldn't be something that people reach for with auto-completion unless they specifically understand the interactions with branch prediction. Using conditional moves can easily make code *slower* by preventing the CPU from speculating past the condition due to the data dependency.
- Although currently `core::hint` only contains no-ops, this isn't a hard rule (for example `unreachable_unchecked` is a bit of a gray area). The documentation only status that the module contains "hints to compiler that affects how code should be emitted or optimized". This is consistent with what `select_unpredictable` does.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2025
…enton

Rollup of 11 pull requests

Successful merges:

 - rust-lang#138972 (std: Fix build for NuttX targets)
 - rust-lang#139177 (Use -C target-cpu=z13 on s390x vector test)
 - rust-lang#139511 (libtest: Pass the test's panic payload as Option instead of Result)
 - rust-lang#139605 (update ```miniz_oxide``` to 0.8.8)
 - rust-lang#139618 (compiletest: Make `SUGGESTION` annotations viral)
 - rust-lang#139677 (Fix profiler_builtins build script to handle full path to profiler lib)
 - rust-lang#139683 (Use `with_native_path` for Windows)
 - rust-lang#139695 (compiletest: consistently use `camino::{Utf8Path,Utf8PathBuf}` throughout)
 - rust-lang#139710 (Move `args` into `std::sys`)
 - rust-lang#139721 (End all lines in src/stage0 with trailing newline)
 - rust-lang#139726 (Move `select_unpredictable` to the `hint` module)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2025
…enton

Rollup of 10 pull requests

Successful merges:

 - rust-lang#138972 (std: Fix build for NuttX targets)
 - rust-lang#139177 (Use -C target-cpu=z13 on s390x vector test)
 - rust-lang#139511 (libtest: Pass the test's panic payload as Option instead of Result)
 - rust-lang#139605 (update ```miniz_oxide``` to 0.8.8)
 - rust-lang#139618 (compiletest: Make `SUGGESTION` annotations viral)
 - rust-lang#139677 (Fix profiler_builtins build script to handle full path to profiler lib)
 - rust-lang#139683 (Use `with_native_path` for Windows)
 - rust-lang#139710 (Move `args` into `std::sys`)
 - rust-lang#139721 (End all lines in src/stage0 with trailing newline)
 - rust-lang#139726 (Move `select_unpredictable` to the `hint` module)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f1d0b9c into rust-lang:master Apr 13, 2025
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2025
Rollup merge of rust-lang#139726 - Amanieu:select_unpredictable_hint, r=dtolnay

Move `select_unpredictable` to the `hint` module

There has been considerable discussion in both the ACP (rust-lang/libs-team#468) and tracking issue (rust-lang#133962) about whether the `bool::select_unpredictable` method should be in `core::hint` instead.

I believe this is the right move for the following reasons:
- The documentation explicitly says that it is a hint, not a codegen guarantee.
- `bool` doesn't have a corresponding `select` method, and I don't think we should be adding one.
- This shouldn't be something that people reach for with auto-completion unless they specifically understand the interactions with branch prediction. Using conditional moves can easily make code *slower* by preventing the CPU from speculating past the condition due to the data dependency.
- Although currently `core::hint` only contains no-ops, this isn't a hard rule (for example `unreachable_unchecked` is a bit of a gray area). The documentation only status that the module contains "hints to compiler that affects how code should be emitted or optimized". This is consistent with what `select_unpredictable` does.
@rustbot rustbot added this to the 1.88.0 milestone Apr 13, 2025
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 19, 2025
… r=dtolnay

Move `select_unpredictable` to the `hint` module

There has been considerable discussion in both the ACP (rust-lang/libs-team#468) and tracking issue (rust-lang#133962) about whether the `bool::select_unpredictable` method should be in `core::hint` instead.

I believe this is the right move for the following reasons:
- The documentation explicitly says that it is a hint, not a codegen guarantee.
- `bool` doesn't have a corresponding `select` method, and I don't think we should be adding one.
- This shouldn't be something that people reach for with auto-completion unless they specifically understand the interactions with branch prediction. Using conditional moves can easily make code *slower* by preventing the CPU from speculating past the condition due to the data dependency.
- Although currently `core::hint` only contains no-ops, this isn't a hard rule (for example `unreachable_unchecked` is a bit of a gray area). The documentation only status that the module contains "hints to compiler that affects how code should be emitted or optimized". This is consistent with what `select_unpredictable` does.
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 19, 2025
…enton

Rollup of 10 pull requests

Successful merges:

 - rust-lang#138972 (std: Fix build for NuttX targets)
 - rust-lang#139177 (Use -C target-cpu=z13 on s390x vector test)
 - rust-lang#139511 (libtest: Pass the test's panic payload as Option instead of Result)
 - rust-lang#139605 (update ```miniz_oxide``` to 0.8.8)
 - rust-lang#139618 (compiletest: Make `SUGGESTION` annotations viral)
 - rust-lang#139677 (Fix profiler_builtins build script to handle full path to profiler lib)
 - rust-lang#139683 (Use `with_native_path` for Windows)
 - rust-lang#139710 (Move `args` into `std::sys`)
 - rust-lang#139721 (End all lines in src/stage0 with trailing newline)
 - rust-lang#139726 (Move `select_unpredictable` to the `hint` module)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API 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