Only memoize const fn calls during const eval #66866
Conversation
Miri and other engines may want to execute the function in order to detect UB inside of them.
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/librustc_mir/const_eval.rs
Outdated
| // 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -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( | |||
There was a problem hiding this comment.
Does this function need to live inside the Miri engine at all? Looks like most everything it does, const_eval.rs could also do.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I'm going to open a separate PR for pulling out all CTFE code to where it is actually needed
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Well I was gonna do a full review again, maybe that's the only one
There was a problem hiding this comment.
Okay. Then please just add a FIXME in this PR.
|
r=me with that FIXME added. |
RalfJung
added
S-waiting-on-author
|
@bors p=1 (fixing toolstate) |
Co-Authored-By: Ralf Jung <post@ralfj.de>
|
@bors r=RalfJung |
|
📌 Commit af8f141 has been approved by |
bors
added
S-waiting-on-bors
Only memoize const fn calls during const eval Miri and other engines may want to execute the function in order to detect UB inside of them. r? @RalfJung
|
☀️ Test successful - checks-azure |
Tested on commit rust-lang/rust@6d77e45. Direct link to PR: <rust-lang/rust#66866> 💔 miri on windows: test-fail → build-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra). 💔 miri on linux: test-fail → build-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).
Miri and other engines may want to execute the function in order to detect UB inside of them.
r? @RalfJung