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.

Sign up for GitHub

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

Only memoize const fn calls during const eval #66866

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
735a610
Only memoize const fn calls during const eval
oli-obk Nov 29, 2019
f15197e
`find_fn` -> `find_mir_or_eval_fn` rename
oli-obk Nov 30, 2019
55840b0
Don't repeat the `is_const_fn_raw` check
oli-obk Nov 30, 2019
af8f141
Update src/librustc_mir/interpret/terminator.rs
oli-obk Dec 4, 2019
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
14 changes: 14 additions & 0 deletions src/librustc_mir/const_eval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,20 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
_unwind: Option<mir::BasicBlock> // unwinding is not supported in consts
) -> InterpResult<'tcx, Option<&'mir mir::Body<'tcx>>> {
debug!("eval_fn_call: {:?}", instance);
oli-obk marked this conversation as resolved.
Show resolved Hide resolved

// 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.
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing the other thing you check for, longer-term, is that there are no relocations in the arguments, so no memory outside of the call could be modified from inside.

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 we want to do that. Otherwise every single const fn call without relocations will get monomorphized, likely causing a significant increase in memory usage (e.g. if you call i32::abs in a loop over 0..10000, you'll get a cached query for every single call).

While the same thing is true for different ZSTs being passed to generic functions, I think that's less extreme since the user needs to write down each type anyway

if ecx.tcx.is_const_fn_raw(instance.def.def_id()) &&
args.iter().all(|a| a.layout.is_zst())
{
let gid = GlobalId { instance, promoted: None };
ecx.eval_const_fn_call(gid, ret)?;
return Ok(None);
}

// 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
Expand Down
14 changes: 1 addition & 13 deletions src/librustc_mir/interpret/terminator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,18 +284,6 @@ 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)? {
Some(body) => body,
Expand Down Expand Up @@ -463,7 +451,7 @@ 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(
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