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

Conversation

@oli-obk
Copy link
Contributor

oli-obk commented Nov 29, 2019

Miri and other engines may want to execute the function in order to detect UB inside of them.

r? @RalfJung

Miri and other engines may want to execute the function in order to detect UB inside of them.
@RalfJung

This comment was marked as off-topic.

Copy link
Member

RalfJung commented Nov 29, 2019

(Very strange, I only got a notification for being assigned here but not for being mentioned in the OP... GitHub emails are fairly unreliable recently. :/ )

@oli-obk

This comment was marked as off-topic.

Copy link
Contributor Author

oli-obk commented Nov 29, 2019

Testing something, ping @eddyb

// 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.

This comment has been minimized.

Copy link
@eddyb

eddyb Nov 29, 2019

Member

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.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Nov 30, 2019

Author Contributor

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

src/librustc_mir/const_eval.rs Outdated Show resolved Hide resolved
@@ -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(

This comment has been minimized.

Copy link
@RalfJung

RalfJung Nov 29, 2019

Member

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

This comment has been minimized.

Copy link
@RalfJung

RalfJung Nov 29, 2019

Member

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).

This comment has been minimized.

Copy link
@oli-obk

oli-obk Nov 30, 2019

Author Contributor

I'm going to open a separate PR for pulling out all CTFE code to where it is actually needed

This comment has been minimized.

Copy link
@RalfJung

RalfJung Nov 30, 2019

Member

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?

This comment has been minimized.

Copy link
@oli-obk

oli-obk Nov 30, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 2, 2019

Member

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

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 2, 2019

r=me with that FIXME added.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 4, 2019

@bors p=1 (fixing toolstate)

Co-Authored-By: Ralf Jung <post@ralfj.de>
@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Dec 4, 2019

@bors r=RalfJung

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2019

📌 Commit af8f141 has been approved by RalfJung

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2019

⌛️ Testing commit af8f141 with merge 6d77e45...

bors added a commit that referenced this pull request Dec 4, 2019
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
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2019

☀️ Test successful - checks-azure
Approved by: RalfJung
Pushing 6d77e45 to master...

@bors bors added the merged-by-bors label Dec 4, 2019
@bors bors merged commit af8f141 into rust-lang:master Dec 4, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr #20191204.35 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 4, 2019

📣 Toolstate changed by #66866!

Tested on commit 6d77e45.
Direct link to PR: #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).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Dec 4, 2019
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.