Use only ty::Unevaluated<'tcx, ()> in type system #98588

b-naber · 2022-06-27T18:23:39Z

r? @lcnr

rust-highfive · 2022-06-27T18:23:43Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured in const_evaluatable.rs

cc @lcnr

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

compiler/rustc_const_eval/src/interpret/operand.rs

RalfJung · 2022-06-27T18:43:25Z

compiler/rustc_const_eval/src/interpret/operand.rs

                let val =
                    self.subst_from_current_frame_and_normalize_erasing_regions(constant.literal)?;
+                debug!(?val);


I'd prefer if we wouldn't sprinkle debug all over the codebase. That makes it super hard to debug the things I want to look into because all information drowns in noise.

At least this should be made trace -- we only want to see this when we are at maximal verbosity level.

RalfJung · 2022-06-27T18:44:10Z

compiler/rustc_const_eval/src/interpret/operand.rs

@@ -632,7 +635,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
            }
            ty::ConstKind::Unevaluated(uv) => {
                let instance = self.resolve(uv.def, uv.substs)?;
-                Ok(self.eval_to_allocation(GlobalId { instance, promoted: uv.promoted })?.into())
+                Ok(self.eval_to_allocation(GlobalId { instance, promoted: None })?.into())


Why does this make sense? It might be a promoted after all...

Also can't this call uneval_to_op now?

this function should be replaced with a function fn ty_const_to_value instead of going to an Operand in one go. I think we should require the caller to normalize unevaluated type system constants and we just always return TooGeneric for ty::Unevaluated here.

Can you explain why we would want to do that? It's doesn't make sense to me why we should first create a ConstValue when we only only want an Operand (we would call eval_to_allocation for creating the ConstValue anyway).

RalfJung · 2022-06-27T18:45:57Z

compiler/rustc_const_eval/src/interpret/operand.rs

@@ -645,6 +648,16 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
        }
    }

+    /// Tries to evaluate an unevaluated constant from the MIR (and not the type-system).


I don't understand this comment. The type of the argument is ty::Unevaluated, that sounds like a type system thing to me?

we use ty::Unevaluated for both type system constants, in which case promoted is () and unevaluated mir constants, in which case promoted is Option<Promoted>.

It is true that this means that ty isn't really the right module for this but idk where else to put it 😆

compiler/rustc_codegen_cranelift/src/constant.rs

lcnr

maybe we want to instead represent ConstantKind::Unevaluated completely without ty::Unevaluated but instead inline the fields 🤔

any shared eval code should then again take def_id, substs, promoted separately. This also avoids the use of ty::X for stuff only used in the mir. not 100% sure what's the best design here.

lcnr · 2022-06-28T10:45:16Z

compiler/rustc_borrowck/src/type_check/mod.rs

                ConstantKind::Ty(ct) => match ct.kind() {
-                    ty::ConstKind::Unevaluated(uv) => Some(uv),
+                    ty::ConstKind::Unevaluated(uv) => Some(uv.expand()),
                    _ => None,
                },


this can never be an inline const, can just return None there.

please change this match to be exhaustive

Doing this causes the test in src/test/ui/inline-const/const-expr-lifetime-err.rs to compile successfully

hmm we should lower inline consts to mir constants, not ty constants 🤔 that's a bug during inline const lowering then

still relevant

compiler/rustc_codegen_cranelift/src/constant.rs

lcnr · 2022-06-28T10:50:53Z

compiler/rustc_codegen_ssa/src/mir/constant.rs

-            mir::ConstantKind::Ty(ct) => ct,
+        let uv = match ct {
+            mir::ConstantKind::Ty(ct) => match ct.kind() {
+                ty::ConstKind::Unevaluated(uv) => uv.expand(),


again only ConstKind::Value should be possible

No, we can still have ConstKind::Unevaluated here.

that seems wrong? the constant should be evaluated in monomorphize 🤔

maybe we should return an Err here if this fails to evaluate

rust/compiler/rustc_trait_selection/src/traits/query/normalize.rs

Lines 327 to 328 in 4045ce6

let constant = constant.try_super_fold_with(self)?;

Ok(constant.eval(self.infcx.tcx, self.param_env))

It's also not clear to me why we would want to error here if we can't evaluate yet, this is called all the time during typecheck, isn't it?

no, the normalize query isn't used during typeck, it's only used afterwards.

i guess it's fine to keep this for now 🤷 will be easier to fix this once your pr has landed

compiler/rustc_codegen_ssa/src/mir/constant.rs

lcnr · 2022-06-28T10:56:34Z

compiler/rustc_const_eval/src/interpret/operand.rs

@@ -632,7 +635,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
            }
            ty::ConstKind::Unevaluated(uv) => {
                let instance = self.resolve(uv.def, uv.substs)?;
-                Ok(self.eval_to_allocation(GlobalId { instance, promoted: uv.promoted })?.into())
+                Ok(self.eval_to_allocation(GlobalId { instance, promoted: None })?.into())


this function should be replaced with a function fn ty_const_to_value instead of going to an Operand in one go. I think we should require the caller to normalize unevaluated type system constants and we just always return TooGeneric for ty::Unevaluated here.

lcnr · 2022-06-28T11:27:13Z

compiler/rustc_monomorphize/src/polymorphize.rs

+            ty::ConstKind::Unevaluated(ty::Unevaluated { def, substs, promoted })
+                if matches!(
+                    self.tcx.def_kind(def.did),
+                    DefKind::AnonConst | DefKind::InlineConst


it can be InlineConst? that shouldn't be the case

still relevant

compiler/rustc_monomorphize/src/polymorphize.rs

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs

lcnr · 2022-06-28T11:30:59Z

compiler/rustc_trait_selection/src/traits/query/normalize.rs

-            mir::ConstantKind::Val(_, _) => constant.try_super_fold_with(self)?,
+            mir::ConstantKind::Val(_, _) | mir::ConstantKind::Unevaluated(..) => {
+                constant.try_super_fold_with(self)?
+            }


this try_fold_mir_const can now stop with the weird ConstantKind::Ty . The default impl of that method should now be good enough

Don't understand what you mean here, we def do need try_fold_mir_const here afaict, since this is where we convert valtrees to constvals.

I think we should do that conversion lazily. keep the mir constant as ConstantKind::Ty(ConstKind::Value(..)) until we actually need it

still relevant

compiler/rustc_typeck/src/check/wfcheck.rs

bors · 2022-06-29T18:48:46Z

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

rustbot · 2022-06-30T14:14:29Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in const_evaluatable.rs

cc @lcnr

b-naber · 2022-06-30T14:16:29Z

Still getting errors in the tests when implementing some of your suggestions, @lcnr.

Trying to find which changes are responsible for the failure, but since running the test set is slow on my machine, I'll use CI.

compiler/rustc_borrowck/src/renumber.rs

b-naber · 2022-06-30T15:23:17Z

What is wrong in the commit that removes visit_const? I'd guess that we can't remove the default impl for super_constant? But how would we in general visit ConstantKind::Ty if we remove visit_const?

lcnr · 2022-07-01T14:20:00Z

But how would we in general visit ConstantKind::Ty if we remove visit_const?

not at all imo 😁 what visitor would want to only look at ConstantKind::Ty but not at ConstantKind::Unevaluated or the constants contained in the types of locals?

b-naber · 2022-07-01T14:40:42Z

But how would we in general visit ConstantKind::Ty if we remove visit_const?

not at all imo 😁 what visitor would want to only look at ConstantKind::Ty but not at ConstantKind::Unevaluated or the constants contained in the types of locals?

I mean that's true, but what should the default impl for super_constant look like?

I don't like it at all that these super methods in the mir visitor aren't required in the impl, you can super easily shoot yourself in the foot as I'm doing right now. So if we don't have visitconst we cannot traverse ConstantKind::Ty in super_constant. What are we supposed to do there instead? Just do nothing? Since we're visiting ty in ConstantKind::Val and ConstantKind::Unevaluated it seeems to me we should call something like TyConstantKind::Ty(ct) => ct.visit_with(self), but mir::Visitors aren't TypeVisitors. To me this just seems inconsistent.

rust-timer · 2022-09-16T14:06:34Z

Queued 1a5b8c954e6c7b563df9c4b135bab9d8ffda9003 with parent 22f6aec, future comparison URL.

rust-timer · 2022-09-16T16:10:27Z

Finished benchmarking commit (1a5b8c954e6c7b563df9c4b135bab9d8ffda9003): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.6%	[0.7%, 2.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-1.3%, -0.4%]	6
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-5.3%, -1.3%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-1.3%	[-1.3%, -1.3%]	1
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	-1.3%	[-1.3%, -1.3%]	1

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

lcnr · 2022-09-16T16:17:17Z

looking good 👍

@bors r+ rollup=never p=10 somewhat bitrotty

bors · 2022-09-16T16:17:19Z

📌 Commit d77248e has been approved by lcnr

It is now in the queue for this repository.

bors · 2022-09-17T03:04:25Z

⌛ Testing commit d77248e with merge c524c7d...

bors · 2022-09-17T05:45:22Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing c524c7d to master...

rust-timer · 2022-09-17T06:52:23Z

Finished benchmarking commit (c524c7d): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-0.6%, -0.6%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.9%	[4.9%, 4.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-4.3%, -4.3%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.2%]	1
All ❌✅ (primary)	-	-	0

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

Use only ty::Unevaluated<'tcx, ()> in type system r? `@lcnr`

RalfJung · 2022-11-15T09:15:38Z

compiler/rustc_const_eval/src/interpret/operand.rs

+                // NOTE: We evaluate to a `ValTree` here as a check to ensure
+                // we're working with valid constants, even though we never need it.
                let instance = self.resolve(uv.def, uv.substs)?;
-                Ok(self.eval_to_allocation(GlobalId { instance, promoted: uv.promoted })?.into())
+                let cid = GlobalId { instance, promoted: None };
+                let _valtree = self
+                    .tcx
+                    .eval_to_valtree(self.param_env.and(cid))?
+                    .unwrap_or_else(|| bug!("unable to create ValTree for {:?}", uv));
+


This is a strange check. Why would CTFE care whether the constant is a valid valtree? What if uv has a type that does not even allow valtree construction, like a raw pointer?

Oh, this is specifically for mir::ConstantKind::Ty, which I guess must always be valtree-constructible.

But then IMO it would make most sense to have a function that evaluates a ty::Const to a Valtree, and always go through that. Doesn't that exist as a query or so?

But then IMO it would make most sense to have a function that evaluates a ty::Const to a Valtree, and always go through that.

I will do that in #104317

rust-highfive assigned lcnr Jun 27, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2022

b-naber commented Jun 27, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/operand.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jun 27, 2022

View reviewed changes

bjorn3 reviewed Jun 27, 2022

View reviewed changes

compiler/rustc_codegen_cranelift/src/constant.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Jun 27, 2022

View reviewed changes

compiler/rustc_codegen_cranelift/src/constant.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

lcnr reviewed Jun 28, 2022

View reviewed changes

b-naber force-pushed the valtrees-cleanup branch from 2fb9d38 to 46d1e13 Compare June 30, 2022 14:14

lcnr reviewed Jun 30, 2022

View reviewed changes

compiler/rustc_borrowck/src/renumber.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

b-naber force-pushed the valtrees-cleanup branch from 46d1e13 to 8cf507b Compare June 30, 2022 14:57

This comment has been minimized.

Sign in to view

b-naber force-pushed the valtrees-cleanup branch 2 times, most recently from bb2451e to 68369fd Compare July 1, 2022 13:39

This comment has been minimized.

Sign in to view

b-naber force-pushed the valtrees-cleanup branch 2 times, most recently from 6e40c69 to 1e04998 Compare July 1, 2022 14:02

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 16, 2022

lcnr added A-const-generics Area: const generics (parameters and arguments) F-adt_const_params `#![feature(adt_const_params)]` labels Sep 16, 2022

This comment was marked as outdated.

Sign in to view

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 17, 2022

bors merged commit c524c7d into rust-lang:master Sep 17, 2022

rustbot added this to the 1.65.0 milestone Sep 17, 2022

This was referenced Sep 17, 2022

Add mir building test directory #101428

Merged

Stop using Reveal::All before borrowck #101478

Closed

WIP: more enum layout optimizations #101819

Closed

EarlyBinder prevent misuse #101901

Merged

b-naber deleted the valtrees-cleanup branch September 17, 2022 06:58

tedinski mentioned this pull request Oct 14, 2022

Update Kani to Rust nightly-2022-10-11 model-checking/kani#1788

Merged

4 tasks

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Oct 23, 2022

Auto merge of rust-lang#98588 - b-naber:valtrees-cleanup, r=lcnr

6214e08

Use only ty::Unevaluated<'tcx, ()> in type system r? `@lcnr`

jruderman mentioned this pull request Nov 9, 2022

ICE "only ConstKind::Param should be encountered here" #104209

Closed

RalfJung reviewed Nov 15, 2022

View reviewed changes

matthiaskrgr mentioned this pull request Dec 4, 2022

ICE -Zpolymorphize=on "collection encountered polymorphic constant: Unevaluated(UnevaluatedConst" #105249

Open

matthiaskrgr mentioned this pull request Apr 13, 2024

ICE: Couldn't evaluate constant const #123893

Open

	let constant = constant.try_super_fold_with(self)?;
	Ok(constant.eval(self.infcx.tcx, self.param_env))

Use only ty::Unevaluated<'tcx, ()> in type system #98588

Use only ty::Unevaluated<'tcx, ()> in type system #98588

Conversation

b-naber commented Jun 27, 2022

rust-highfive commented Jun 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

lcnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jun 29, 2022

rustbot commented Jun 30, 2022

b-naber commented Jun 30, 2022

This comment has been minimized.

This comment has been minimized.

b-naber commented Jun 30, 2022 • edited Loading

This comment has been minimized.

This comment has been minimized.

lcnr commented Jul 1, 2022 • edited Loading

b-naber commented Jul 1, 2022 • edited Loading

rust-timer commented Sep 16, 2022

rust-timer commented Sep 16, 2022

Overall result: ❌✅ regressions and improvements - no action needed

Footnotes

lcnr commented Sep 16, 2022

bors commented Sep 16, 2022

This comment was marked as outdated.

This comment was marked as outdated.

bors commented Sep 17, 2022

bors commented Sep 17, 2022

rust-timer commented Sep 17, 2022

Overall result: ❌✅ regressions and improvements - no action needed

Footnotes

Choose a reason for hiding this comment

RalfJung Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Jun 30, 2022 •

edited

Loading

lcnr commented Jul 1, 2022 •

edited

Loading

b-naber commented Jul 1, 2022 •

edited

Loading

RalfJung Nov 15, 2022 •

edited

Loading