-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add new intrinsic is_constant
and optimize pow
#114390
Changes from 10 commits
abeda62
4aa0608
2605ea7
f569cea
1b75669
79016f8
4f31013
cf9406f
be35381
c1adc30
87b5cea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2489,6 +2489,51 @@ extern "rust-intrinsic" { | |
/// constructing an empty slice) is returned. | ||
#[rustc_nounwind] | ||
pub fn option_payload_ptr<T>(arg: *const Option<T>) -> *const T; | ||
|
||
/// Returns whether the argument's value is statically known at | ||
/// compile-time. | ||
/// | ||
/// This is useful when there is a way of writing the code that will | ||
/// be *faster* when some variables have known values, but *slower* | ||
/// in the general case: an `if is_val_statically_known(var)` can be used | ||
/// to select between these two variants. The `if` will be optimized away | ||
/// and only the desired branch remains. | ||
/// | ||
/// Formally speaking, this function non-deterministically returns `true` | ||
/// or `false`, and the caller has to ensure sound behavior for both cases. | ||
/// In other words, the following code has *Undefined Behavior*: | ||
/// | ||
/// ```rust | ||
/// if !is_val_statically_known(0) { unreachable_unchecked(); } | ||
/// ``` | ||
/// | ||
/// This also means that the following code's behavior is unspecified; it | ||
/// may panic, or it may not: | ||
/// | ||
/// ```rust,no_run | ||
/// assert_eq!(is_val_statically_known(0), black_box(is_val_statically_known(0))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either way, the behavior is unspecified, even without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about entirely removing black_box from the example? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good |
||
/// ``` | ||
Comment on lines
+2506
to
+2515
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both doc tests fail when ran with You need to add Here is an example that passes the tests:
|
||
/// | ||
/// Unsafe code may not rely on `is_val_statically_known` returning any | ||
/// particular value, ever. However, the compiler will generally make it | ||
/// return `true` only if the value of the argument is actually known. | ||
Centri3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// | ||
/// When calling this in a `const fn`, both paths must be semantically | ||
/// equivalent, that is, the result of the `true` branch and the `false` | ||
/// branch must return the same value and have the same side-effects *no | ||
/// matter what*. | ||
Comment on lines
+2521
to
+2524
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are they allowed to differ in panics? E.g. if an invalid input is handed to a safe function then it may either panic or return garbage and this differs between the branches? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding onto this, are panic messages/locations allowed to differ (if it always panics with the same inputs in CTFE/runtime)? They do in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say they must either both panic or neither panic. But the exact panic location is not relevant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That should be documented then. Though I think it might be tricky to uphold. Would it be possible to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could (in debug mode?) fork the const evaluator and run both paths and compare the result 🙃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's the same with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think there's any reason not to. Custom panic messages would be a nice addition also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to add, for the benefit of those reading in the future, that the core library already intentionally gives different panic messages depending on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's used for a couple other things as well, like having certain assertions only in the run-time version of some low-level functions ( |
||
#[rustc_const_unstable(feature = "is_val_statically_known", issue = "none")] | ||
#[rustc_nounwind] | ||
#[cfg(not(bootstrap))] | ||
pub fn is_val_statically_known<T>(arg: T) -> bool; | ||
} | ||
|
||
// FIXME: Seems using `unstable` here completely ignores `rustc_allow_const_fn_unstable` | ||
// and thus compiling stage0 core doesn't work. | ||
#[rustc_const_stable(feature = "is_val_statically_known", since = "never")] | ||
#[cfg(bootstrap)] | ||
pub const unsafe fn is_val_statically_known<T: ~const crate::marker::Destruct>(_: T) -> bool { | ||
false | ||
Centri3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// Some functions are defined here because they accidentally got made | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2039,26 +2039,49 @@ macro_rules! int_impl { | |
without modifying the original"] | ||
#[inline] | ||
#[rustc_inherit_overflow_checks] | ||
#[rustc_allow_const_fn_unstable(is_val_statically_known)] | ||
pub const fn pow(self, mut exp: u32) -> Self { | ||
if exp == 0 { | ||
return 1; | ||
} | ||
let mut base = self; | ||
let mut acc = 1; | ||
|
||
while exp > 1 { | ||
if (exp & 1) == 1 { | ||
acc = acc * base; | ||
// SAFETY: This path has the same behavior as the other. | ||
if unsafe { intrinsics::is_val_statically_known(self) } | ||
&& self > 0 | ||
&& (self & (self - 1) == 0) | ||
{ | ||
let power_used = match self.checked_ilog2() { | ||
Some(v) => v, | ||
// SAFETY: We just checked this is a power of two. and above zero. | ||
None => unsafe { core::hint::unreachable_unchecked() }, | ||
}; | ||
// So it panics. Have to use `overflowing_mul` to efficiently set the | ||
// result to 0 if not. | ||
#[cfg(debug_assertions)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is incorrect to use Without To correctly check for overflow, test to see if the final result is less than 1. If that is the case use Although it isn't necessary, you can make the code less verbose by using |
||
{ | ||
_ = power_used * exp; | ||
} | ||
let (num_shl, overflowed) = power_used.overflowing_mul(exp); | ||
let fine = !overflowed | ||
& (num_shl < (mem::size_of::<Self>() * 8) as u32); | ||
(1 << num_shl) * fine as Self | ||
} else { | ||
if exp == 0 { | ||
return 1; | ||
} | ||
let mut base = self; | ||
let mut acc = 1; | ||
|
||
while exp > 1 { | ||
if (exp & 1) == 1 { | ||
acc = acc * base; | ||
} | ||
exp /= 2; | ||
base = base * base; | ||
} | ||
exp /= 2; | ||
base = base * base; | ||
} | ||
|
||
// since exp!=0, finally the exp must be 1. | ||
// Deal with the final bit of the exponent separately, since | ||
// squaring the base afterwards is not necessary and may cause a | ||
// needless overflow. | ||
acc * base | ||
// since exp!=0, finally the exp must be 1. | ||
// Deal with the final bit of the exponent separately, since | ||
// squaring the base afterwards is not necessary and may cause a | ||
// needless overflow. | ||
acc * base | ||
} | ||
} | ||
|
||
/// Calculates the quotient of Euclidean division of `self` by `rhs`. | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -33,6 +33,21 @@ fn main() { | |||||
assert_eq!(intrinsics::likely(false), false); | ||||||
assert_eq!(intrinsics::unlikely(true), true); | ||||||
|
||||||
let mut saw_true = false; | ||||||
let mut saw_false = false; | ||||||
|
||||||
Comment on lines
+37
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
just a nit to make it visually more clear that these are all together testing one thing EDIT: hm I tried to remove the empty line, not sure if github supports that... |
||||||
for _ in 0..50 { | ||||||
if unsafe { intrinsics::is_val_statically_known(0) } { | ||||||
saw_true = true; | ||||||
} else { | ||||||
saw_false = true; | ||||||
} | ||||||
} | ||||||
assert!( | ||||||
saw_true && saw_false, | ||||||
"`is_val_statically_known` failed to return both true and false. Congrats, you won the lottery!" | ||||||
); | ||||||
|
||||||
intrinsics::forget(Bomb); | ||||||
|
||||||
let _v = intrinsics::discriminant_value(&Some(())); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// #[cfg(bootstrap)] | ||
// ignore-stage1 | ||
// compile-flags: --crate-type=lib -Zmerge-functions=disabled | ||
|
||
#![feature(core_intrinsics)] | ||
|
||
use std::intrinsics::is_val_statically_known; | ||
|
||
pub struct A(u32); | ||
pub enum B { | ||
Ye(u32), | ||
} | ||
|
||
#[inline] | ||
pub fn _u32(a: u32) -> i32 { | ||
if unsafe { is_val_statically_known(a) } { 1 } else { 0 } | ||
} | ||
|
||
// CHECK-LABEL: @_u32_true( | ||
#[no_mangle] | ||
pub fn _u32_true() -> i32 { | ||
// CHECK: ret i32 1 | ||
_u32(1) | ||
} | ||
|
||
// CHECK-LABEL: @_u32_false( | ||
#[no_mangle] | ||
pub fn _u32_false(a: u32) -> i32 { | ||
// CHECK: ret i32 0 | ||
_u32(a) | ||
} | ||
|
||
#[inline] | ||
pub fn _bool(b: bool) -> i32 { | ||
if unsafe { is_val_statically_known(b) } { 3 } else { 2 } | ||
} | ||
|
||
// CHECK-LABEL: @_bool_true( | ||
#[no_mangle] | ||
pub fn _bool_true() -> i32 { | ||
// CHECK: ret i32 3 | ||
_bool(true) | ||
} | ||
|
||
// CHECK-LABEL: @_bool_false( | ||
#[no_mangle] | ||
pub fn _bool_false(b: bool) -> i32 { | ||
// CHECK: ret i32 2 | ||
_bool(b) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we need to use
immediate_llvm_type
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what's the difference between the two? I don't know much about LLVM, hence the long wait for getting CI to pass...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm_type is the type used to pass arguments of that Rust type between functions; immediate_llvm_type is the type used for LLVM SSA values of that Rust type.