Move select_unpredictable to the hint module by Amanieu · Pull Request #139726 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation9 Commits1 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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.

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

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 added the T-libs

Relevant to the library team, which will review and decide on the PR/issue.

label

Apr 12, 2025

This comment has been minimized.

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

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.

📌 Commit 5d90ccb has been approved by dtolnay

It is now in the queue for this repository.

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

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.

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

bors added a commit to rust-lang-ci/rust that referenced this pull request

Apr 13, 2025

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.

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

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.