Change Thir to lazily create constants #94876

b-naber · 2022-03-12T09:22:12Z

To allow AbstractConsts to work with the previous thir changes we made and those we want to make, i.e. to avoid problems due to ValTree and ConstValue conversions, we instead switch to a thir representation for constants that allows us to lazily create constants.

r? @oli-obk

b-naber · 2022-03-12T09:34:03Z

compiler/rustc_mir_build/src/build/expr/as_temp.rs

@@ -70,7 +70,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                    local_decl.local_info =
                        Some(Box::new(LocalInfo::StaticRef { def_id, is_thread_local: true }));
                }
-                ExprKind::Literal { const_id: Some(def_id), .. } => {
+                ExprKind::NamedConst { def_id, .. } => {


This might be problematic. I'm not completely sure whether we need any other variants here. Also don't know what adding constants to local_info here does later in codegen.

we previously also did this for ExprKind::ConstParam, would have to figure out what it's used for myself 😆 think you can just keep the current behavior and add a FIXME for now

Suggested change

ExprKind::NamedConst { def_id, .. } => {

ExprKind::NamedConst { def_id, .. } | ExprKind::ConstParam { def_id, .. } => {

we also want this for ExprKind::ConstParam: please add this as a test:

#![feature(adt_const_params)] #![allow(incomplete_features)] #[derive(PartialEq, Eq)] struct Yikes; impl Yikes { fn mut_self(&mut self) {} } fn foo<const YIKES: Yikes>() { YIKES.mut_self() } fn main() { foo::<{ Yikes }>() }

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=fd11458a9e567c54fbb2fb43c15c9f56

compiler/rustc_mir_build/src/thir/cx/expr.rs

b-naber · 2022-03-15T11:34:13Z

This is ready for review now. Shouldn't be reviewed commit by commit probably.

oli-obk · 2022-03-15T11:50:16Z

This is ready for review now. Shouldn't be reviewed commit by commit probably.

I guess squash the commits in that case?

lcnr

a few nits

compiler/rustc_middle/src/thir/visit.rs

compiler/rustc_mir_build/src/build/expr/as_constant.rs

compiler/rustc_middle/src/thir.rs

compiler/rustc_mir_build/src/thir/cx/expr.rs

lcnr · 2022-03-15T11:53:38Z

compiler/rustc_mir_build/src/build/expr/as_temp.rs

@@ -70,7 +70,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                    local_decl.local_info =
                        Some(Box::new(LocalInfo::StaticRef { def_id, is_thread_local: true }));
                }
-                ExprKind::Literal { const_id: Some(def_id), .. } => {
+                ExprKind::NamedConst { def_id, .. } => {


we previously also did this for ExprKind::ConstParam, would have to figure out what it's used for myself 😆 think you can just keep the current behavior and add a FIXME for now

Suggested change

ExprKind::NamedConst { def_id, .. } => {

ExprKind::NamedConst { def_id, .. } | ExprKind::ConstParam { def_id, .. } => {

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs

b-naber · 2022-03-15T15:23:05Z

Decided to keep the thir::Pat code as is for now.

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs

lcnr

Alright, am pretty happy with this PR now.

I do dislike ScalarLiteral, it feels like a pretty bad name 🤔 can't think of a better name myself, so please take 1-2 min to think of one and if not we can merge it as is 😆

lcnr · 2022-03-22T13:01:11Z

compiler/rustc_mir_build/src/build/expr/as_temp.rs

@@ -70,7 +70,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                    local_decl.local_info =
                        Some(Box::new(LocalInfo::StaticRef { def_id, is_thread_local: true }));
                }
-                ExprKind::Literal { const_id: Some(def_id), .. } => {
+                ExprKind::NamedConst { def_id, .. } => {


we also want this for ExprKind::ConstParam: please add this as a test:

#![feature(adt_const_params)] #![allow(incomplete_features)] #[derive(PartialEq, Eq)] struct Yikes; impl Yikes { fn mut_self(&mut self) {} } fn foo<const YIKES: Yikes>() { YIKES.mut_self() } fn main() { foo::<{ Yikes }>() }

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=fd11458a9e567c54fbb2fb43c15c9f56

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs

lcnr · 2022-03-22T13:37:54Z

@bors try @rust-timer queue

rust-timer · 2022-03-22T13:37:56Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

lcnr · 2022-03-23T12:51:03Z

interesting 🤔 I would be surprised if these changes mattered, even if keeping them would probably have been preferable.

let's try it again, maybe it was spurious in some way?

@bors try @rust-timer queue

rust-timer · 2022-03-23T12:51:05Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-03-23T12:51:12Z

⌛ Trying commit 96050421395ae1c3a75dc0a883e0d9317f918195 with merge 7827037e4b0cf03a1f99c331fa9ef74dd062fbea...

bors · 2022-03-23T14:27:42Z

☀️ Try build successful - checks-actions
Build commit: 7827037e4b0cf03a1f99c331fa9ef74dd062fbea (7827037e4b0cf03a1f99c331fa9ef74dd062fbea)

rust-timer · 2022-03-23T14:27:44Z

Queued 7827037e4b0cf03a1f99c331fa9ef74dd062fbea with parent cd2da4d, future comparison URL.

rust-timer · 2022-03-23T16:52:49Z

Finished benchmarking commit (7827037e4b0cf03a1f99c331fa9ef74dd062fbea): comparison url.

Summary: This benchmark run shows 11 relevant improvements 🎉 but 5 relevant regressions 😿 to instruction counts.

Arithmetic mean of relevant regressions: 0.4%
Arithmetic mean of relevant improvements: -1.6%
Arithmetic mean of all relevant changes: -1.0%
Largest improvement in instruction counts: -2.8% on full builds of keccak check
Largest regression in instruction counts: 0.5% on incr-unchanged builds of deeply-nested-multi check

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

b-naber · 2022-03-23T16:57:17Z

hm this is very weird. How can it be that the number of calls of a query differ in perf runs? Is this a bug in rustc_perf? (cc @Mark-Simulacrum)

compiler/rustc_middle/src/query/mod.rs

lcnr · 2022-03-24T11:25:24Z

@bors r+ rollup=never

bors · 2022-03-24T11:25:25Z

📌 Commit 19041d9 has been approved by lcnr

bors · 2022-03-24T12:50:09Z

⌛ Testing commit 19041d9 with merge 8d8135f...

bors · 2022-03-24T15:17:20Z

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

bors · 2022-03-24T15:18:40Z

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

rust-timer · 2022-03-25T04:38:06Z

Finished benchmarking commit (8d8135f): comparison url.

Summary: This benchmark run shows 32 relevant improvements 🎉 but 13 relevant regressions 😿 to instruction counts.

Arithmetic mean of relevant regressions: 1.6%
Arithmetic mean of relevant improvements: -1.0%
Arithmetic mean of all relevant changes: -0.3%
Largest improvement in instruction counts: -2.8% on full builds of keccak check
Largest regression in instruction counts: 3.5% on full builds of tuple-stress check

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

pnkfelix · 2022-03-30T18:38:42Z

Visiting for weekly performance triage.

The changes to the primary benchmarks are almost universally green here.

The secondary benchmarks are more of a mixed bag, but I think we can accept a 3.5% regression on tuple-stress given the overall benefits.

@rustbot label: +perf-regression-triaged

rust-highfive assigned oli-obk Mar 12, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 12, 2022

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

This comment has been minimized.

Sign in to view

b-naber force-pushed the thir-abstract-const-changes branch from 73a22ab to 9e73a91 Compare March 12, 2022 09:26

b-naber commented Mar 12, 2022

View reviewed changes

compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved

oli-obk reviewed Mar 12, 2022

View reviewed changes

compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

oli-obk reviewed Mar 12, 2022

View reviewed changes

compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved

b-naber force-pushed the thir-abstract-const-changes branch from a2ef0d4 to f7ff7aa Compare March 15, 2022 11:24

b-naber changed the title ~~[WIP] Thir abstract const changes~~ Change Thir to lazily create constants Mar 15, 2022

b-naber force-pushed the thir-abstract-const-changes branch from f7ff7aa to 1f24795 Compare March 15, 2022 11:33

lcnr reviewed Mar 15, 2022

View reviewed changes

b-naber force-pushed the thir-abstract-const-changes branch from e13f6b6 to 07f5641 Compare March 15, 2022 15:22

This comment has been minimized.

Sign in to view

b-naber force-pushed the thir-abstract-const-changes branch from e2e84c3 to c962c81 Compare March 15, 2022 15:44

This comment has been minimized.

Sign in to view

b-naber force-pushed the thir-abstract-const-changes branch from c962c81 to 8e21280 Compare March 15, 2022 16:32

lcnr reviewed Mar 16, 2022

View reviewed changes

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs Show resolved Hide resolved

lcnr reviewed Mar 16, 2022

View reviewed changes

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

b-naber force-pushed the thir-abstract-const-changes branch from c98ec1a to b3d71d3 Compare March 16, 2022 08:55

lcnr reviewed Mar 22, 2022

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 22, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 23, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 23, 2022

lcnr reviewed Mar 23, 2022

View reviewed changes

compiler/rustc_middle/src/query/mod.rs Outdated Show resolved Hide resolved

b-naber force-pushed the thir-abstract-const-changes branch from 9605042 to ccfbe52 Compare March 23, 2022 18:42

This comment has been minimized.

Sign in to view

dont use a query for lit_to_constant

19041d9

b-naber force-pushed the thir-abstract-const-changes branch from ccfbe52 to 19041d9 Compare March 23, 2022 19:18

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 Mar 24, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 24, 2022

bors merged commit 8d8135f into rust-lang:master Mar 24, 2022

rustbot added this to the 1.61.0 milestone Mar 24, 2022

bors mentioned this pull request Mar 24, 2022

Fix constants not getting dropped if part of a diverging expression #94775

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 30, 2022

b-naber deleted the thir-abstract-const-changes branch August 3, 2022 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Thir to lazily create constants #94876

Change Thir to lazily create constants #94876

b-naber commented Mar 12, 2022 •

edited

This comment has been minimized.

b-naber Mar 12, 2022

lcnr Mar 15, 2022

lcnr Mar 22, 2022

This comment has been minimized.

b-naber commented Mar 15, 2022

oli-obk commented Mar 15, 2022

lcnr left a comment

lcnr Mar 15, 2022

b-naber commented Mar 15, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lcnr left a comment

lcnr Mar 22, 2022

lcnr commented Mar 22, 2022

rust-timer commented Mar 22, 2022

lcnr commented Mar 23, 2022

rust-timer commented Mar 23, 2022

bors commented Mar 23, 2022

bors commented Mar 23, 2022

rust-timer commented Mar 23, 2022

rust-timer commented Mar 23, 2022

b-naber commented Mar 23, 2022

This comment has been minimized.

lcnr commented Mar 24, 2022

bors commented Mar 24, 2022

bors commented Mar 24, 2022

bors commented Mar 24, 2022

bors commented Mar 24, 2022

rust-timer commented Mar 25, 2022

pnkfelix commented Mar 30, 2022 •

edited

	ExprKind::NamedConst { def_id, .. } => {
	ExprKind::NamedConst { def_id, .. } \| ExprKind::ConstParam { def_id, .. } => {

Change Thir to lazily create constants #94876

Change Thir to lazily create constants #94876

Conversation

b-naber commented Mar 12, 2022 • edited

This comment has been minimized.

b-naber Mar 12, 2022

Choose a reason for hiding this comment

lcnr Mar 15, 2022

Choose a reason for hiding this comment

lcnr Mar 22, 2022

Choose a reason for hiding this comment

This comment has been minimized.

b-naber commented Mar 15, 2022

oli-obk commented Mar 15, 2022

lcnr left a comment

Choose a reason for hiding this comment

lcnr Mar 15, 2022

Choose a reason for hiding this comment

b-naber commented Mar 15, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lcnr left a comment

Choose a reason for hiding this comment

lcnr Mar 22, 2022

Choose a reason for hiding this comment

lcnr commented Mar 22, 2022

rust-timer commented Mar 22, 2022

lcnr commented Mar 23, 2022

rust-timer commented Mar 23, 2022

bors commented Mar 23, 2022

bors commented Mar 23, 2022

rust-timer commented Mar 23, 2022

rust-timer commented Mar 23, 2022

b-naber commented Mar 23, 2022

This comment has been minimized.

lcnr commented Mar 24, 2022

bors commented Mar 24, 2022

bors commented Mar 24, 2022

bors commented Mar 24, 2022

bors commented Mar 24, 2022

rust-timer commented Mar 25, 2022

pnkfelix commented Mar 30, 2022 • edited

b-naber commented Mar 12, 2022 •

edited

pnkfelix commented Mar 30, 2022 •

edited