Skip to content

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
20 changes: 20 additions & 0 deletions src/libcore/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,26 @@ extern "rust-intrinsic" {
/// Executes a breakpoint trap, for inspection by a debugger.
pub fn breakpoint();

/// Returns `true` during constant evaluation and `false` otherwise.
///
/// # Safety
///
/// This intrinsic allows breaking [referential transparency] in `const fn`
/// and is therefore `unsafe`.
///
/// Code that uses this intrinsic must be extremely careful to ensure that
/// `const fn`s remain referentially-transparent independently of when they
/// are evaluated.
Copy link
Member

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.

Copy link
Contributor

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

Copy link
Member

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.

Copy link
Contributor Author

@gnzlbg gnzlbg Sep 23, 2019

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?

How would that run-time feature detection thing be implemented ? 😉

Copy link
Contributor

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?

Copy link
Contributor Author

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 an if cfg!(miri) { false } branch in it, but it is not usable inside const fns. To be able to use the macro in const fns we'd need something like this, the macro uses inline assembly internally..

Copy link
Member

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 ? wink

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 in const 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 do

#[cfg(miri)]
const fn is_miri() { true }
#[cfg(not(miri))]
const fn is_miri() { false }

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 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 from const fns, either const fns 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 call is_x86_feature_detected! from a const 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.

Copy link
Contributor Author

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 fns scope and the plan is indeed to actually support inline assembly in const fns at some point, that is.

Copy link
Member

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 fns scope and the plan is indeed to actually support inline assembly in const fns at some point, that is.

😱: https://github.com/CraneStation/cranelift/issues/444#issuecomment-531574394

///
/// The Rust compiler assumes that it is sound to replace a call to a `const
/// fn` with the result produced by evaluating it at compile-time. If
/// evaluating the function at run-time were to produce a different result,
/// or have any other observable side-effects, the behavior is undefined.
///
/// [referential transparency]: https://en.wikipedia.org/wiki/Referential_transparency
#[cfg(not(boostrap_stdarch_ignore_this))]
pub fn is_const_eval() -> bool;

/// The size of a type in bytes.
///
/// More specifically, this is the offset in bytes between successive
Expand Down
3 changes: 3 additions & 0 deletions src/librustc_codegen_llvm/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,9 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
}

},
"is_const_eval" => {
self.const_bool(false)
},
"fadd_fast" | "fsub_fast" | "fmul_fast" | "fdiv_fast" | "frem_fast" => {
match float_type_width(arg_tys[0]) {
Some(_width) =>
Expand Down
6 changes: 5 additions & 1 deletion src/librustc_mir/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

Won't this mean that it returns true in Miri, which seems odd (for now, anyway)?

This should query a machine flag to determine the intrinsic's return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't this mean that it returns true in Miri, which seems odd (for now, anyway)?

This returns true in miri. Were it to return false, miri would fail trying to do something it doesn't support.

Copy link
Contributor Author

@gnzlbg gnzlbg Sep 23, 2019

Choose a reason for hiding this comment

The 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 cfg!(miri) which can be used to the same effect.

This should query a machine flag to determine the intrinsic's return value.

Does cfg!(miri) do this?

EDIT: users that want to do something else for miri can just use cfg(miri) on top of this, instead of making the result of this intrinsic be target dependent.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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 is_const_eval must return false.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 is_const_eval must return false.

I could cfg(miri) is_const_eval to always return false, but then miri won't be able to execute any code for which is_const_eval makes sense using.

}
let tp_ty = substs.type_at(0);
Ok(match name {
"type_name" => {
let alloc = type_name::alloc_type_name(tcx, tp_ty);
Expand Down Expand Up @@ -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" |
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -557,8 +557,8 @@ impl Qualif for IsNotPromotable {
| "saturating_add"
| "saturating_sub"
| "transmute"
| "is_const_eval"
=> return true,

_ => {}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc_typeck/check/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem) {
"rustc_peek" => (1, vec![param(0)], param(0)),
"panic_if_uninhabited" => (1, Vec::new(), tcx.mk_unit()),
"init" => (1, Vec::new(), param(0)),
"is_const_eval" => (0, Vec::new(), tcx.types.bool),
"uninit" => (1, Vec::new(), param(0)),
"forget" => (1, vec![param(0)], tcx.mk_unit()),
"transmute" => (2, vec![ param(0) ], param(1)),
Expand Down
16 changes: 16 additions & 0 deletions src/test/codegen/intrinsics/is_const_eval.rs
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()
}
11 changes: 11 additions & 0 deletions src/test/ui/intrinsics/is_const_eval.rs
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() };
Copy link
Contributor Author

@gnzlbg gnzlbg Sep 22, 2019

Choose a reason for hiding this comment

The 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 feature(const_fn) is not enabled!).

Even if we were to fix #61495, this would still "just work" behind the const_fn feature. That would allow it to "silently" be used in APIs exposed by libstd. I'm not sure how to fix that.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with @oli-obk. Why would just "just fork" with const_fn? That gate won't make non-const-fn callable, and all intrinsics (except for a whitelist) should be considered non-const-fn.

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 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() };
assert_eq!(X, true);
assert_eq!(y, false);
}