-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Add is_const_eval intrinsic #64683
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
Changes from 1 commit
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
|
||
} |
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.
Uh oh!
There was an error while loading. Please reload this page.
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