-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 is_const_eval intrinsic #64683
Add is_const_eval intrinsic #64683
Changes from all commits
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 |
---|---|---|
|
@@ -46,8 +46,11 @@ crate fn eval_nullary_intrinsic<'tcx>( | |
def_id: DefId, | ||
substs: SubstsRef<'tcx>, | ||
) -> InterpResult<'tcx, &'tcx ty::Const<'tcx>> { | ||
let tp_ty = substs.type_at(0); | ||
let name = &*tcx.item_name(def_id).as_str(); | ||
if name == "is_const_eval" { | ||
return Ok(ty::Const::from_bool(tcx, true)); | ||
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. Won't this mean that it returns This should query a machine flag to determine the intrinsic's return value. 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.
This returns 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 don't think this is odd at all given that miri already provides
Does EDIT: users that want to do something else for miri can just use 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. The difference is that miri is more like codegen and not like const eval in its semantics. Cfgs flags are their own world independent of the runtime 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. Miri is implementing Rust's run-time semantics as much as it can. So If you actually want to check "is this a restricted interpreted environment that does not support SIMD", then you should pick an appropriate name for the intrinsic. 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 could |
||
} | ||
let tp_ty = substs.type_at(0); | ||
oli-obk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Ok(match name { | ||
"type_name" => { | ||
let alloc = type_name::alloc_type_name(tcx, tp_ty); | ||
|
@@ -94,6 +97,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |
|
||
let intrinsic_name = &self.tcx.item_name(instance.def_id()).as_str()[..]; | ||
match intrinsic_name { | ||
"is_const_eval" | | ||
"min_align_of" | | ||
"pref_align_of" | | ||
"needs_drop" | | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
// compile-flags: -C no-prepopulate-passes | ||
|
||
#![crate_type = "lib"] | ||
#![feature(core_intrinsics)] | ||
|
||
use std::intrinsics::is_const_eval; | ||
|
||
// CHECK-LABEL: @is_const_eval_test | ||
#[no_mangle] | ||
pub unsafe fn is_const_eval_test() -> bool { | ||
// CHECK: %0 = alloca i8, align 1 | ||
// CHECK: store i8 0, i8* %0, align 1 | ||
// CHECK: %2 = trunc i8 %1 to i1 | ||
// CHECK: ret i1 %2 | ||
is_const_eval() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// run-pass | ||
|
||
#![feature(core_intrinsics)] | ||
use std::intrinsics::is_const_eval; | ||
|
||
fn main() { | ||
const X: bool = unsafe { is_const_eval() }; | ||
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. So AFAICT the only remaining issue is that this currently works due to #61495 allowing all intrinsics to run in const-contexts (notice that Even if we were to fix #61495, this would still "just work" behind 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. If it is fixed as I imagine it, the const fn feature gate won't have an effect on it. It would only permit a whitelist of intrinsics and forbid all others outside unleash mode 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. Agreed with @oli-obk. Why would just "just fork" 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. I think the "why" issue is already resolved in the updated OP description and the new test that shows an use case that works with this, but wouldn't work without it. |
||
let y = unsafe { is_const_eval() }; | ||
oli-obk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert_eq!(X, true); | ||
assert_eq!(y, false); | ||
gnzlbg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
// run-pass | ||
// only-x86_64 | ||
// compile-flags: -Zunleash-the-miri-inside-of-you | ||
|
||
#![feature(core_intrinsics)] | ||
use std::intrinsics::is_const_eval; | ||
use std::arch::x86_64::*; | ||
use std::mem::transmute; | ||
|
||
const fn eq(x: [i32; 4], y: [i32; 4]) -> bool { | ||
if unsafe { is_const_eval() } { | ||
x[0] == y[0] && x[1] == y[1] && x[2] == y[2] && x[3] == y[3] | ||
} else { | ||
unsafe { | ||
let x = _mm_loadu_si128(&x as *const _ as *const _); | ||
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. This is essentially what I worried about. Such code will never ever be accepted by rustc AFAIK. If you want it for experimentation, that seems fine but ultimately pointless. Writing a const qualification that could accept this would be the motherload of unsoundness dangers. My suggestion with an intrinsic, that calls one of two functions given to it depending on the context, would be much more manageable 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. Long version: In contrast to C++, we statically prove that your const fn is sane. Of course we also have escape hatches via unsafe, but that's a different story. What your example has is an if condition that works differently whether you are const evaluating or codegenning. Now, this is not a problem by itself, but the problem arises with 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. I see. Yes, the "boolean intrinsic" approach implemented here would require the constant evaluator to only try to prove things about the branches that constant evaluation actually can reach. I can see how that could get nasty. IIUC, what you are proposing is something like: mod intrinsics {
pub fn const_eval_select<T, U, V>(called_in_const: T, called_at_rt: U) -> V
where T: const Fn() -> V, U: Fn() -> V;
} that can be used to rewrite the example above as follows: const fn eq_ct(x: [i32; 4], y: [i32; 4]) -> bool {
x[0] == y[0] && x[1] == y[1] && x[2] == y[2] && x[3] == y[3]
}
fn eq_rt(x: [i32; 4], y: [i32; 4]) -> bool {
let x = _mm_loadu_si128(&x as *const _ as *const _);
let y = _mm_loadu_si128(&y as *const _ as *const _);
let r = _mm_cmpeq_epi32(x, y);
let r = _mm_movemask_ps(transmute(r) );
r == 0b1111
}
const fn eq(x: [i32; 4], y: [i32; 4]) -> bool {
const_eval_select(|| eq_ct(x, y), || eq_rt(x, y))
// unclear to me how to make these closures work well here
} ? Is that correct? Maybe you could post about it in the tracking issue (rust-lang/const-eval#7) ? Your last comment there appeared to be in agreement with using a "boolean intrinsic", but it does have quite a bit of issues, so maybe we can move the discussion there. 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. While closures would be neat, they seem an unnecessary complication of such a low level function mod intrinsics {
pub fn const_eval_select<ARG, F, G, RET>(arg: ARG called_in_const: F, called_at_rt: G) -> RET;
} you could then use it as const fn eq_ct((x, y): ([i32; 4], [i32; 4])) -> bool {
x[0] == y[0] && x[1] == y[1] && x[2] == y[2] && x[3] == y[3]
}
fn eq_rt((x, y): ([i32; 4], [i32; 4])) -> bool {
let x = _mm_loadu_si128(&x as *const _ as *const _);
let y = _mm_loadu_si128(&y as *const _ as *const _);
let r = _mm_cmpeq_epi32(x, y);
let r = _mm_movemask_ps(transmute(r) );
r == 0b1111
}
const fn eq(x: [i32; 4], y: [i32; 4]) -> bool {
const_eval_select((x, y), eq_ct, eq_rt)
// unclear to me how to make these closures work well here
} 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. This sounds great. So how do I implement this? IIUC, I just add an intrinsic to typeck like I did here, and implement it with the "const semantics" in const-eval, and then add a different implementation for it in the llvm backend, and that's it ? 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 can't imagine this is feasible, we'd need to analyze the "runtime code" and figure out if it does something non-const friendly, but we can't really tell much about that because the point of this feature is to be able to call SIMD intrinsics or inline assembly, which is essentially stuf we cannot reason about. I'd be fine with saying that this intrinsic is 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. Oh that's not what I meant. I just mean that
The content of the functions is indeed left to the user as per the docs you already provided 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. Ah, gotcha. Hm, what about just saying: mod intrinsics {
pub fn const_eval_select<ARG, F, G, RET>(
arg: ARG called_in_const: F, called_at_rt: G
) -> RET
where F: FnOnce(ARG) -> RET, G: FnOnce(ARG) -> RET
} ? That way, 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 thought we wouldn't go for closures, to keep things simple? Then it would just be pub fn const_eval_select<ARG, RET>(
arg: ARG,
called_in_const: fn(ARG) -> RET,
called_at_rt: fn(ARG) -> RET,
) -> RET 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. function pointers are not the same thing as the zst arguments and won't do us any favours here. The intrinsic would not be able to figure out which function to call and would have to insert function pointer calls and we can't have const function pointers. While all solvable problems, I'd rather not go down that road I mean you can add
That's not checked, it could be a closure. Thus it needs an extra check to make sure only functions are used as generic arguments |
||
let y = _mm_loadu_si128(&y as *const _ as *const _); | ||
let r = _mm_cmpeq_epi32(x, y); | ||
let r = _mm_movemask_ps(transmute(r) ); | ||
r == 0b1111 | ||
} | ||
} | ||
} | ||
|
||
fn main() { | ||
const X: bool = eq([0, 1, 2, 3], [0, 1, 2, 3]); | ||
assert_eq!(X, true); | ||
let x = eq([0, 1, 2, 3], [0, 1, 2, 3]); | ||
assert_eq!(x, true); | ||
|
||
const Y: bool = eq([0, 1, 2, 3], [0, 1, 3, 2]); | ||
assert_eq!(Y, false); | ||
let y = eq([0, 1, 2, 3], [0, 1, 3, 2]); | ||
assert_eq!(y, false); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:11:17 | ||
| | ||
LL | if unsafe { is_const_eval() } { | ||
| ^^^^^^^^^^^^^^^ | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:11:5 | ||
| | ||
LL | / if unsafe { is_const_eval() } { | ||
LL | | x[0] == y[0] && x[1] == y[1] && x[2] == y[2] && x[3] == y[3] | ||
LL | | } else { | ||
LL | | unsafe { | ||
... | | ||
LL | | } | ||
LL | | } | ||
| |_____^ | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:26:5 | ||
| | ||
LL | assert_eq!(X, true); | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:26:5 | ||
| | ||
LL | assert_eq!(X, true); | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:26:5 | ||
| | ||
LL | assert_eq!(X, true); | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:28:5 | ||
| | ||
LL | assert_eq!(x, true); | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:28:5 | ||
| | ||
LL | assert_eq!(x, true); | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:28:5 | ||
| | ||
LL | assert_eq!(x, true); | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:31:5 | ||
| | ||
LL | assert_eq!(Y, false); | ||
| ^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:31:5 | ||
| | ||
LL | assert_eq!(Y, false); | ||
| ^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:31:5 | ||
| | ||
LL | assert_eq!(Y, false); | ||
| ^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:33:5 | ||
| | ||
LL | assert_eq!(y, false); | ||
| ^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:33:5 | ||
| | ||
LL | assert_eq!(y, false); | ||
| ^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
||
warning: skipping const checks | ||
--> $DIR/is_const_eval_unleash.rs:33:5 | ||
| | ||
LL | assert_eq!(y, false); | ||
| ^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) | ||
|
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.
I find the reference of referential transparency here mostly unhelpful. At least, I would have no idea what exactly const code must (not) do, based on this description.
But also, this does not affect const-qualif. It doesn't actually introduce any referential intransparency. So personally I'd trim this down to "the code must do the same thing for both cases". Which leaves you wonder why this is even added.
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.
My understanding is that this is used in C++ to speed up the run-time code with SIMD and whatnot but that both code paths should observably behave same (for some notion of observational equivalence). https://en.cppreference.com/w/cpp/types/is_constant_evaluated
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.
If this SIMD code used run-time feature detection, couldn't be "just" make that report "no SIMD support" for const-eval to avoid needing any other magic intrinsic?
Either way though, this should be stated in the PR description.
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.
How would that run-time feature detection thing be implemented ? 😉
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.
I thought that already existed for SIMD anyway?
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.
is_x86_feature_detected!
has anif cfg!(miri) { false }
branch in it, but it is not usable insideconst fn
s. To be able to use the macro inconst fn
s we'd need something like this, the macro uses inline assembly internally..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.
Not sure, but I don't think we need this intrinsic for it? (And if we do, the name should be
is_miri
.)Is the problem just that you cannot do
if
inconst fn
? We are accumulating more and more bad hacks to work around that and I'd rather put the line down before we add intrinsics for this reason. You can for example already doThere 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.
I don't think you have understood what the problem is here.
is_x86_feature_detected!
needs to use inline assembly at run-time. If you want to call that fromconst fn
s, eitherconst fn
s need to support inline assembly, or you need to provide a way to do something else at compile time.If you want to "properly" emulate
is_x86_feature_detected
on miri then miri would need to support inline assembly. Right now, we just have a#[cfg(miri)] return false;
that does nothing on miri instead. That could be improved with some "hook" that miri provides for doing run-time feature detection, that calls some function from the "miri run-time" that is implemented using inline assembly or similar. This has nothing to do with the problem being discussed here though, which is how to callis_x86_feature_detected!
from aconst fn
, or more generally, how to be able to call efficient run-time code that uses "stuff that constant evaluation probably won't ever support" like inline assembly by providing an alternative slower implementation for constant evaluation.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.
Unless I completely misunderstood
const fn
s scope and the plan is indeed to actually support inline assembly in const fns at some point, that is.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.
😱: https://github.com/CraneStation/cranelift/issues/444#issuecomment-531574394