Canonicalize inputs to const eval where needed #68505

BenLewis-Seequent · 2020-01-24T07:14:53Z

Canonicalize inputs to const eval, so that they can contain inference variables. Which enables invoking const eval queries even if the current param env has inference variable within it, which can occur during trait selection.

This is a reattempt of #67717, in a far less invasive way.

Fixes #68477

r? @nikomatsakis
cc @eddyb

eddyb

LGTM, but I'll leave the approval to @nikomatsakis.

eddyb · 2020-01-24T18:45:01Z

src/librustc/mir/interpret/queries.rs

+        let mut query_state = OriginalQueryValues::default();
+        let canonical = self.canonicalize_query(&(param_env, substs), &mut query_state);
+        let (param_env, substs) = canonical.value;
+        self.tcx.const_eval_resolve(param_env, def_id, substs, promoted, span)


Wait, I just remembered something: you need to replace the type with the original one, if you want to return the value (since it's not used, you could just opt to return Result<(), ...> for now).
This is because you don't want the canonical variables to stick around in the type.

There's also a related trick I've told @nikomatsakis about before:
The value produced by const-eval shouldn't depend on lifetimes, so we should probably always erase them in the input substs (and param_env even?).
But the type might have relevant lifetimes, which means we should (just like in the canonical case) use the original type along the new value.

I believe both canonicalizing and erasing lifetimes, would produce the correct value, but erasing them would result in perhaps better cache utilization.

Ideally, canonicalization would replace all lifetimes and hence be the same, from a cache perspective, but we may not currently handle things that way (we may treat 'static as special or something...)

I would think erasing lifetimes would also be benefical for const eval queries which don't use canonicalization, i.e. the ones that don't use InferCtxt::const_eval_reolve, although in most cases they already probably have there regions erased. It does look like canonicalize_query, which i'm using, does replace `static lifetimes.

But the type might have relevant lifetimes, which means we should (just like in the canonical case) use the original type along the new value.

Most usage of const-eval is by codegen or inside miri which doesn't need/want lifetimes, and in other cases the caller knows what type it's expecting, so I think it should be up to the callers to the replace the type if they need to. One place I'm unsure about is:

rust/src/librustc_mir_build/hair/pattern/mod.rs

Line 764 in 8bf1758

match self.tcx.const_eval_resolve(

I'm not sure if lifetimes are important here or not, if they are I can replace the type with the node type retrieved above.

nikomatsakis · 2020-01-24T22:51:00Z

Welp this is definitely shorter =)

It seems like we should do a perf run, though, right?

nikomatsakis · 2020-01-24T22:51:07Z

@bors try

bors · 2020-01-24T22:51:19Z

⌛ Trying commit 0f12888bfeab463a0e1278c13ea8869f49bdd559 with merge ed91a52f6d36d06eaf36de547fda2e61d41780af...

nikomatsakis · 2020-01-24T23:11:49Z

@rust-timer queue

rust-timer · 2020-01-24T23:11:50Z

Awaiting bors try build completion

bors · 2020-01-25T01:15:17Z

☀️ Try build successful - checks-azure
Build commit: ed91a52f6d36d06eaf36de547fda2e61d41780af (ed91a52f6d36d06eaf36de547fda2e61d41780af)

rust-timer · 2020-01-25T01:15:19Z

Queued ed91a52f6d36d06eaf36de547fda2e61d41780af with parent c2d141d, future comparison URL.

rust-timer · 2020-01-25T04:49:05Z

Finished benchmarking try commit ed91a52f6d36d06eaf36de547fda2e61d41780af, comparison URL.

BenLewis-Seequent · 2020-01-25T08:35:54Z

Can I get another perf run to see if erasing lifetimes has any effect on performance?

varkor · 2020-01-25T10:12:55Z

@bors try
@rust-timer queue

rust-timer · 2020-01-25T10:12:56Z

Awaiting bors try build completion

bors · 2020-01-25T10:13:07Z

⌛ Trying commit c8e35e3df4016e72bf7eb3a1508a592a80d1f3ae with merge d7a172c31012c265a79fc422a27e2b8e35978ff3...

bors · 2020-01-25T12:41:38Z

☀️ Try build successful - checks-azure
Build commit: d7a172c31012c265a79fc422a27e2b8e35978ff3 (d7a172c31012c265a79fc422a27e2b8e35978ff3)

rust-timer · 2020-01-25T12:41:39Z

Queued d7a172c31012c265a79fc422a27e2b8e35978ff3 with parent 8bf1758, future comparison URL.

BenLewis-Seequent · 2020-01-25T19:56:15Z

Perf numbers looks like just noise.

src/librustc/mir/interpret/queries.rs

src/librustc/ty/sty.rs

BenLewis-Seequent · 2020-01-27T07:52:00Z

This actually now does fix #68477, as we are now erasing lifetimes of all inputs to const-eval, including region inference variables.

Aaron1011 · 2020-01-30T23:16:31Z

What's the status of this PR?

varkor · 2020-01-30T23:33:31Z

This is waiting for a review by @nikomatsakis, who has been at the Mozilla All Hands this week, so I imagine it'll be reviewed next week.

bors · 2020-02-06T01:57:08Z

☔ The latest upstream changes (presumably #68461) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis

This seems good to me. My only hesitation is that I can't quite tell what was going on with the erase-regions question, I guess maybe I need to dig a bit deeper there to form an opinion?

src/librustc/infer/canonical/substitute.rs

src/librustc/mir/interpret/queries.rs

shisoft · 2020-02-11T18:36:42Z

Hi, what's the status of this PR?

nikomatsakis · 2020-02-14T18:05:14Z

I think the status is that we are waiting for @Skinny121 to do the following:

rebase the PR
potentially adjust the return type to hide the erased regions
(optional) refactor the substitute_value codepath

bors · 2020-02-17T01:55:59Z

☔ The latest upstream changes (presumably #67953) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb

Change const eval to just return the value As discussed in rust-lang#68505 (comment), the type of consts shouldn't be returned from const eval queries. r? @eddyb cc @nikomatsakis

…ithin this code path.

BenLewis-Seequent · 2020-02-20T17:32:16Z

Should be ready for code review again. #69181 changed the return type of const eval queries to just the value as requested, as a result the changes to substitute_value are no longer needed as no canonical variables can exist in the resultant ConstValue.

nikomatsakis · 2020-02-24T16:24:35Z

@Skinny121 if I understood correctly, this means my comments about refactoring the substitution code is no longer relevant, but @eddyb's concern around erasing regions is still relevant, right? Any thoughts on fixing that as they suggested?

BenLewis-Seequent · 2020-02-26T06:53:14Z

@nikomatsakis That concern isn't relevant any more as the erased regions don't escape that function, as ConstEvalResult<'tcx> is now Result<ConstValue<'tcx>, ErrorHandled>, which doesn't contain any types/regions.

nikomatsakis · 2020-02-26T15:53:51Z

@bors r+

bors · 2020-02-26T15:53:52Z

📌 Commit ebfa2f4 has been approved by nikomatsakis

bors · 2020-02-28T06:14:59Z

⌛ Testing commit ebfa2f4 with merge bfc32dd...

bors · 2020-02-28T09:32:27Z

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing bfc32dd to master...

rust-highfive assigned nikomatsakis Jan 24, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2020

This comment has been minimized.

Sign in to view

varkor added the F-const_generics `#![feature(const_generics)]` label Jan 24, 2020

eddyb approved these changes Jan 24, 2020

View reviewed changes

eddyb reviewed Jan 24, 2020

View reviewed changes

BenLewis-Seequent force-pushed the canonicalize-const-eval-inputs branch from b9ae8b1 to c8e35e3 Compare January 25, 2020 07:44

eddyb reviewed Jan 25, 2020

View reviewed changes

src/librustc/mir/interpret/queries.rs Show resolved Hide resolved

eddyb reviewed Jan 25, 2020

View reviewed changes

src/librustc/ty/sty.rs Outdated Show resolved Hide resolved

BenLewis-Seequent force-pushed the canonicalize-const-eval-inputs branch from c8e35e3 to b426c05 Compare January 26, 2020 06:37

BenLewis-Seequent mentioned this pull request Jan 26, 2020

Lazy normalization of constants #67890

Closed

Aaron1011 mentioned this pull request Feb 6, 2020

Query-ify Instance::resolve #67797

Merged

nikomatsakis reviewed Feb 7, 2020

View reviewed changes

src/librustc/infer/canonical/substitute.rs Outdated Show resolved Hide resolved

src/librustc/mir/interpret/queries.rs Show resolved Hide resolved

BenLewis-Seequent force-pushed the canonicalize-const-eval-inputs branch from 2e8fe60 to 1961b1b Compare February 14, 2020 19:39

BenLewis-Seequent mentioned this pull request Feb 15, 2020

Change const eval to just return the value #69181

Merged

skinnyBat added 4 commits February 20, 2020 19:44

Canonicalize inputs to const eval where needed.

f77f07c

Erase regions before before performing const eval, to improve caching.

b8523fd

Change FIXME message to indicate plan to handle inference variables w…

ea18cd7

…ithin this code path.

Add regression test for issue 68477.

ebfa2f4

BenLewis-Seequent force-pushed the canonicalize-const-eval-inputs branch from 1961b1b to ebfa2f4 Compare February 20, 2020 17:24

Aaron1011 mentioned this pull request Feb 26, 2020

Normalize all opaque types when converting ParamEnv to Reveal::All #65989

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 28, 2020

bors merged commit bfc32dd into rust-lang:master Feb 28, 2020

bors mentioned this pull request Feb 28, 2020

Split librustc::{traits,infer} to their respective crates #69076

Merged

varkor mentioned this pull request Jul 22, 2020

Tracking issue for const generics (RFC 2000) #44580

Closed

16 tasks

dwrensha mentioned this pull request Oct 14, 2021

thread 'rustc' panicked at 'index out of bounds: the len is 0 but the index is 0' #89868

Closed

Canonicalize inputs to const eval where needed #68505

Canonicalize inputs to const eval where needed #68505

Conversation

BenLewis-Seequent commented Jan 24, 2020 • edited Loading

This comment has been minimized.

eddyb left a comment

Choose a reason for hiding this comment

eddyb Jan 24, 2020

Choose a reason for hiding this comment

nikomatsakis Jan 24, 2020

Choose a reason for hiding this comment

BenLewis-Seequent Jan 25, 2020

Choose a reason for hiding this comment

BenLewis-Seequent Jan 25, 2020 • edited Loading

Choose a reason for hiding this comment

nikomatsakis commented Jan 24, 2020

nikomatsakis commented Jan 24, 2020

bors commented Jan 24, 2020

nikomatsakis commented Jan 24, 2020

rust-timer commented Jan 24, 2020

bors commented Jan 25, 2020

rust-timer commented Jan 25, 2020

rust-timer commented Jan 25, 2020

BenLewis-Seequent commented Jan 25, 2020

varkor commented Jan 25, 2020

rust-timer commented Jan 25, 2020

bors commented Jan 25, 2020

bors commented Jan 25, 2020

rust-timer commented Jan 25, 2020

BenLewis-Seequent commented Jan 25, 2020

BenLewis-Seequent commented Jan 27, 2020

Aaron1011 commented Jan 30, 2020

varkor commented Jan 30, 2020

bors commented Feb 6, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

shisoft commented Feb 11, 2020

nikomatsakis commented Feb 14, 2020 • edited Loading

bors commented Feb 17, 2020

BenLewis-Seequent commented Feb 20, 2020

nikomatsakis commented Feb 24, 2020

BenLewis-Seequent commented Feb 26, 2020

nikomatsakis commented Feb 26, 2020

bors commented Feb 26, 2020

bors commented Feb 28, 2020

bors commented Feb 28, 2020

BenLewis-Seequent commented Jan 24, 2020 •

edited

Loading

BenLewis-Seequent Jan 25, 2020 •

edited

Loading

nikomatsakis commented Feb 14, 2020 •

edited

Loading