Skip to content

core: add instrinsics for llvm.expect.i8-i64 #22883

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

Closed
wants to merge 1 commit into from

Conversation

seanmonstar
Copy link
Contributor

Closes #20579

@rust-highfive
Copy link
Contributor

r? @aturon

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

/// Inform the optimizer that `expected_val` is expected
/// (the most probable) value of val.
#[cfg(stage0)]
pub unsafe fn expect8(val: u8, _expected_val: u8) -> u8 {
Copy link
Member

Choose a reason for hiding this comment

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

I... expect... these aren't actually necessary to do the bootstrap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow? Before i added cfg(stage0) versions, it complained during stage0 that these intrinsics weren't known by the compiler.

Copy link
Member

Choose a reason for hiding this comment

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

These functions aren't called during the bootstrap, so you can have the FFI declarations be #[cfg(not(stage0))] and just omit these altogether. (I.e. have core::intrinsics::expect* not exist in stage0 at all.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see what you mean.

@alexcrichton
Copy link
Member

Currently we are trying to hide the intrinsics module entirely from the standard library's API so in the long run these will not be accessible. (see #22358 (comment) for some more info).

In the past we've been ok adding these form of intrinsics if they're directly used in the standard library and provide tangible benefits. Exposing them as a public API, however, would require an RFC in any case. For now though because these are not used internally and also not exposed through a stable interface, I'm going to close this. I also think that #20579 should live in the RFC repo instead of this repo as it's a general feature request, I'll see if can't get it moved over.

Thanks for the PR though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement branch prediction intrinsics
5 participants