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

Only memoize const fn calls during const eval #66866

Merged
merged 4 commits into from
Dec 4, 2019
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
18 changes: 15 additions & 3 deletions src/librustc_mir/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,20 +328,32 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
false // for now, we don't enforce validity
}

fn find_fn(
fn find_mir_or_eval_fn(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx>],
ret: Option<(PlaceTy<'tcx>, mir::BasicBlock)>,
_unwind: Option<mir::BasicBlock> // unwinding is not supported in consts
) -> InterpResult<'tcx, Option<&'mir mir::Body<'tcx>>> {
debug!("eval_fn_call: {:?}", instance);
debug!("find_mir_or_eval_fn: {:?}", instance);

// Only check non-glue functions
if let ty::InstanceDef::Item(def_id) = instance.def {
// Execution might have wandered off into other crates, so we cannot do a stability-
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
// sensitive check here. But we can at least rule out functions that are not const
// at all.
if !ecx.tcx.is_const_fn_raw(def_id) {
if ecx.tcx.is_const_fn_raw(def_id) {
// If this function is a `const fn` then as an optimization we can query this
// evaluation immediately.
//
// For the moment we only do this for functions which take no arguments
// (or all arguments are ZSTs) so that we don't memoize too much.
if args.iter().all(|a| a.layout.is_zst()) {
let gid = GlobalId { instance, promoted: None };
ecx.eval_const_fn_call(gid, ret)?;
return Ok(None);
}
} else {
// Some functions we support even if they are non-const -- but avoid testing
// that for const fn! We certainly do *not* want to actually call the fn
// though, so be sure we return here.
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
/// nor just jump to `ret`, but instead push their own stack frame.)
/// Passing `dest`and `ret` in the same `Option` proved very annoying when only one of them
/// was used.
fn find_fn(
fn find_mir_or_eval_fn(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx, Self::PointerTag>],
Expand Down
17 changes: 3 additions & 14 deletions src/librustc_mir/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,20 +284,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
ty::InstanceDef::DropGlue(..) |
ty::InstanceDef::CloneShim(..) |
ty::InstanceDef::Item(_) => {
// If this function is a `const fn` then as an optimization we can query this
// evaluation immediately.
//
// For the moment we only do this for functions which take no arguments
// (or all arguments are ZSTs) so that we don't memoize too much.
if self.tcx.is_const_fn_raw(instance.def.def_id()) &&
args.iter().all(|a| a.layout.is_zst())
{
let gid = GlobalId { instance, promoted: None };
return self.eval_const_fn_call(gid, ret);
}

// We need MIR for this fn
let body = match M::find_fn(self, instance, args, ret, unwind)? {
let body = match M::find_mir_or_eval_fn(self, instance, args, ret, unwind)? {
Some(body) => body,
None => return Ok(()),
};
Expand Down Expand Up @@ -463,7 +451,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

/// Evaluate a const function where all arguments (if any) are zero-sized types.
/// The evaluation is memoized thanks to the query system.
fn eval_const_fn_call(
// FIXME: Consider moving this to `const_eval.rs`.
pub (crate) fn eval_const_fn_call(
Copy link
Member

Choose a reason for hiding this comment

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

Does this function need to live inside the Miri engine at all? Looks like most everything it does, const_eval.rs could also do.

Copy link
Member

Choose a reason for hiding this comment

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

Though I feel like that file could really benefit from being split into the basic CTFE machine definition, and the rest... (similar to machine.rs vs eval.rs in Miri).

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'm going to open a separate PR for pulling out all CTFE code to where it is actually needed

Copy link
Member

Choose a reason for hiding this comment

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

All CTFE code? Isn't eval_const_fn_call the only one that's inside interpret/ when it shouldn't be?
I cleaned this up during my refactorings, and I don't think anything besides this fn here sneaked in since then. But I might have missed some PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I was gonna do a full review again, maybe that's the only one

Copy link
Member

Choose a reason for hiding this comment

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

Okay. Then please just add a FIXME in this PR.

oli-obk marked this conversation as resolved.
Show resolved Hide resolved
&mut self,
gid: GlobalId<'tcx>,
ret: Option<(PlaceTy<'tcx, M::PointerTag>, mir::BasicBlock)>,
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine {
false
}

fn find_fn(
fn find_mir_or_eval_fn(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
_instance: ty::Instance<'tcx>,
_args: &[OpTy<'tcx>],
Expand Down