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

Stabilize hint::assert_unchecked #123588

Merged
merged 2 commits into from
Jul 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion library/alloc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@
#![feature(fmt_internals)]
#![feature(fn_traits)]
#![feature(hasher_prefixfree_extras)]
#![feature(hint_assert_unchecked)]
#![feature(inplace_iteration)]
#![feature(iter_advance_by)]
#![feature(iter_next_chunk)]
Expand Down
97 changes: 74 additions & 23 deletions library/core/src/hint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,41 +111,92 @@ pub const unsafe fn unreachable_unchecked() -> ! {

/// Makes a *soundness* promise to the compiler that `cond` holds.
///
/// This may allow the optimizer to simplify things,
/// but it might also make the generated code slower.
/// Either way, calling it will most likely make compilation take longer.
/// This may allow the optimizer to simplify things, but it might also make the generated code
/// slower. Either way, calling it will most likely make compilation take longer.
///
/// This is a situational tool for micro-optimization, and is allowed to do nothing.
/// Any use should come with a repeatable benchmark to show the value
/// and allow removing it later should the optimizer get smarter and no longer need it.
/// You may know this from other places as
/// [`llvm.assume`](https://llvm.org/docs/LangRef.html#llvm-assume-intrinsic) or, in C,
/// [`__builtin_assume`](https://clang.llvm.org/docs/LanguageExtensions.html#builtin-assume).
///
/// The more complicated the condition the less likely this is to be fruitful.
/// For example, `assert_unchecked(foo.is_sorted())` is a complex enough value
/// that the compiler is unlikely to be able to take advantage of it.
/// This promotes a correctness requirement to a soundness requirement. Don't do that without
/// very good reason.
///
/// There's also no need to `assert_unchecked` basic properties of things. For
/// example, the compiler already knows the range of `count_ones`, so there's no
/// benefit to `let n = u32::count_ones(x); assert_unchecked(n <= u32::BITS);`.
/// # Usage
///
/// If ever you're tempted to write `assert_unchecked(false)`, then you're
/// actually looking for [`unreachable_unchecked()`].
/// This is a situational tool for micro-optimization, and is allowed to do nothing. Any use
/// should come with a repeatable benchmark to show the value, with the expectation to drop it
/// later should the optimizer get smarter and no longer need it.
///
/// You may know this from other places
/// as [`llvm.assume`](https://llvm.org/docs/LangRef.html#llvm-assume-intrinsic)
/// or [`__builtin_assume`](https://clang.llvm.org/docs/LanguageExtensions.html#builtin-assume).
/// The more complicated the condition, the less likely this is to be useful. For example,
/// `assert_unchecked(foo.is_sorted())` is a complex enough value that the compiler is unlikely
/// to be able to take advantage of it.
///
/// This promotes a correctness requirement to a soundness requirement.
/// Don't do that without very good reason.
/// There's also no need to `assert_unchecked` basic properties of things. For example, the
/// compiler already knows the range of `count_ones`, so there is no benefit to
/// `let n = u32::count_ones(x); assert_unchecked(n <= u32::BITS);`.
///
/// `assert_unchecked` is logically equivalent to `if !cond { unreachable_unchecked(); }`. If
/// ever you are tempted to write `assert_unchecked(false)`, you should instead use
/// [`unreachable_unchecked()`] directly.
///
/// # Safety
///
/// `cond` must be `true`. It's immediate UB to call this with `false`.
/// `cond` must be `true`. It is immediate UB to call this with `false`.
///
/// # Example
///
/// ```
/// use core::hint;
///
/// /// # Safety
/// ///
/// /// `p` must be nonnull and valid
/// pub unsafe fn next_value(p: *const i32) -> i32 {
/// // SAFETY: caller invariants guarantee that `p` is not null
/// unsafe { hint::assert_unchecked(!p.is_null()) }
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a pretty weird example. Do we not have something better? We even have to defend the example later in the documentation as being not real world.

One example that feels more illustrative is guaranteeing that the returned integer is in-bounds for some slice. For example, binary search tells LLVM that the indexes it returns are in-bounds of the length. Replicating binary search here probably is too much, but replicating a simpler algorithm (e.g., &[(u32, usize)] where you search for a particular needle and every usize is guaranteed to be an index in the array -- so before returning it you want to tell LLVM that) feels like a better example.

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 didn't think there was much purpose in showing a real example of how it may be used in practice since we say elsewhere that you should only use this after benchmarking, and that a smarter optimizer may be able to figure it out at some point. So rather than showing an real usecase that might go away, I figured a dummy example of how it can actually affect codegen may be more useful.

But I am happy to change here as needed, maybe I can rework this example to make its intent more obvious.

Copy link
Member

Choose a reason for hiding this comment

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

I like this example. It's not necessarily real world, but I do think it is effective at demonstrating the principle here in a pretty small and easily digestible code snippet.

///
/// if p.is_null() {
/// return -1;
/// } else {
/// // SAFETY: caller invariants guarantee that `p` is valid
/// unsafe { *p + 1 }
/// }
/// }
/// ```
///
/// Without the `assert_unchecked`, the above function produces the following with optimizations
/// enabled:
///
/// ```asm
/// next_value:
/// test rdi, rdi
/// je .LBB0_1
/// mov eax, dword ptr [rdi]
/// inc eax
/// ret
/// .LBB0_1:
/// mov eax, -1
/// ret
/// ```
///
/// Adding the assertion allows the optimizer to remove the extra check:
///
/// ```asm
/// next_value:
/// mov eax, dword ptr [rdi]
/// inc eax
/// ret
/// ```
///
/// This example is quite unlike anything that would be used in the real world: it is redundant
/// to put an an assertion right next to code that checks the same thing, and dereferencing a
/// pointer already has the builtin assumption that it is nonnull. However, it illustrates the
/// kind of changes the optimizer can make even when the behavior is less obviously related.
#[track_caller]
#[inline(always)]
#[doc(alias = "assume")]
#[track_caller]
#[unstable(feature = "hint_assert_unchecked", issue = "119131")]
#[rustc_const_unstable(feature = "const_hint_assert_unchecked", issue = "119131")]
#[stable(feature = "hint_assert_unchecked", since = "CURRENT_RUSTC_VERSION")]
#[rustc_const_stable(feature = "hint_assert_unchecked", since = "CURRENT_RUSTC_VERSION")]
pub const unsafe fn assert_unchecked(cond: bool) {
// SAFETY: The caller promised `cond` is true.
unsafe {
Expand Down
2 changes: 1 addition & 1 deletion library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -959,7 +959,7 @@ extern "rust-intrinsic" {
/// not be used if the invariant can be discovered by the optimizer on its
/// own, or if it does not enable any significant optimizations.
///
/// This intrinsic does not have a stable counterpart.
/// The stabilized version of this intrinsic is [`core::hint::assert_unchecked`].
#[rustc_const_stable(feature = "const_assume", since = "1.77.0")]
#[rustc_nounwind]
#[unstable(feature = "core_intrinsics", issue = "none")]
Expand Down
1 change: 0 additions & 1 deletion library/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@
#![feature(const_fmt_arguments_new)]
#![feature(const_hash)]
#![feature(const_heap)]
#![feature(const_hint_assert_unchecked)]
#![feature(const_index_range_slice_index)]
#![feature(const_int_from_str)]
#![feature(const_intrinsic_copy)]
Expand Down
1 change: 0 additions & 1 deletion library/std/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,6 @@
#![feature(fmt_internals)]
#![feature(hasher_prefixfree_extras)]
#![feature(hashmap_internals)]
#![feature(hint_assert_unchecked)]
#![feature(ip)]
#![feature(maybe_uninit_slice)]
#![feature(maybe_uninit_uninit_array)]
Expand Down
6 changes: 1 addition & 5 deletions tests/ui/consts/const-assert-unchecked-ub.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
#![feature(hint_assert_unchecked)]
#![feature(const_hint_assert_unchecked)]

const _: () = unsafe {
let n = u32::MAX.count_ones();
std::hint::assert_unchecked(n < 32); //~ ERROR evaluation of constant value failed
};

fn main() {
}
fn main() {}
2 changes: 1 addition & 1 deletion tests/ui/consts/const-assert-unchecked-ub.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0080]: evaluation of constant value failed
--> $DIR/const-assert-unchecked-ub.rs:6:5
--> $DIR/const-assert-unchecked-ub.rs:3:5
|
LL | std::hint::assert_unchecked(n < 32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `assume` called with `false`
Expand Down
Loading