Add Finalize statement to make deaggregation "reversible" by storing all information in MIR #96043

oli-obk · 2022-04-14T09:27:57Z

cc @compiler-errors (Due to the additional information encodable for temporaries, once we stop emitting aggregates during mir building, this should improve diagnostics in borrowck, and especially around opaque types)

This PR can stand on its own, but I'm not sure its valuable on its own. I'm planning to continue moving the deaggregation pass earlier, until it runs right after mir building. Then I will remove it and make mir building generate deaggregated MIR

rust-highfive · 2022-04-14T09:28:00Z

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2022-04-14T09:28:01Z

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

oli-obk · 2022-04-14T09:35:21Z

r? rust-lang/wg-mir-opt

oli-obk · 2022-04-14T09:35:53Z

hmm that didn't work

r? @wesleywiser

cc @rust-lang/wg-mir-opt

JakobDegen · 2022-04-14T09:53:24Z

compiler/rustc_middle/src/mir/visit.rs

+                    StatementKind::Finalize(place) => {
+                        self.visit_place(
+                            place,
+                            PlaceContext::MutatingUse(MutatingUseContext::Store),


In MIR like this:

_1.0 = val; Finalize(_1);

Won't MaybeLiveLocals now report _1 as being dead before the finalize?

If the semantics of this were that it was just a read, we could have this be a NonMutatingUseContext::Inspect or something like that.

JakobDegen · 2022-04-14T09:58:16Z

compiler/rustc_const_eval/src/interpret/step.rs

+            Finalize(place) => {
+                if M::enforce_validity(self) {
+                    let place = self.eval_place(**place)?;
+                    // Invariant: when `Finalize` is invoked on a place, that place is fully valid
+                    self.validate_operand(&self.place_to_op(&place)?)?;
+                }
+            }


Does this implement the semantics documented below? This does not look like it reports a mutation/store for the place

🤦 this is unreachable, I'm erasing all Finalize before going into optimized_mir, because it's not useful after that

RemoveFinalize looks like it's currently gated on mir_opt_level > 0

Ah, I remember why I added it. The const propagator runs on MIR that still has Finalize statements

Should Miri run this, or not?

enforce_validity is anyway false for CTFE.

I think that's just unsound? If you have a variable of type MaybeUninit that is initialized but the value it is initialized to is unknown, then validation will be like "sure uninit is fine here" but actually the optimization of replacing it by uninit is wrong.

There is a big difference between "this memory is definitely uninit" and "we don't know the contents of this memory", and it sounds like const prop is conflating these two things?

Yea, we knew this wasn't going to scale to some types. We'd need a different kind of marker for unknown data, but I also don't know if we can do that without complicating the interpreter. Maybe const prop should be its own thing... but it's also sad if we duplicate work.

const prop does symbolic evaluation. It is not surprising that a concrete interpreter is not up for the task.

It might be possible to generalize parts of the interpreter to support symbolic and concrete evaluation, but we probably don't want that for the full thing. (const prop doesn't hit all parts of the interpreter anyway.)

Currently, what do we do to prevent soundness issues like the one I described?

We avoid propagating most aggregates.

I think the right level of code sharing would be to call the various math operations manually and not reuse any of the functions that evaluate a mir statement or rvalue

Yeah, I agree. Basically operator.rs and cast.rs.

compiler/rustc_middle/src/mir/mod.rs

JakobDegen · 2022-04-14T10:17:13Z

src/test/mir-opt/retag.array_casts.SimplifyCfg-elaborate-drops.after.mir

+        Deinit(_13);                     // scope 5 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+        (_13.0: &usize) = move _14;      // scope 5 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+        Retag((_13.0: &usize));          // scope 5 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+        (_13.1: &usize) = move _18;      // scope 5 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+        Retag((_13.1: &usize));          // scope 5 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+        Finalize(_13);                   // scope 5 at $SRC_DIR/core/src/macros/mod.rs:LL:COL


The fact that we now emit the additional retags is interesting. I think this is the behavior that we always should have had, but I'm not sure. cc @RalfJung

jup, there are now some minor diagnostics changes in miri where we point to more precise locations

Miri currently only retags bare references, not references inside aggregrates. This change here "surfaced" more bare references, hence there are more retags.

compiler/rustc_mir_transform/src/remove_finalize.rs

oli-obk · 2022-04-14T21:16:19Z

@bors try @rust-timer queue

rust-timer · 2022-04-14T21:16:20Z

Awaiting bors try build completion.

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

bors · 2022-04-14T21:16:27Z

⌛ Trying commit d316ab5b1fa8d310b9d6601f40bb9d2ee6b1d9c3 with merge 8d16576297d5544e5e8cccfe1ba734fccfe63dc9...

bors · 2022-04-14T22:44:25Z

☀️ Try build successful - checks-actions
Build commit: 8d16576297d5544e5e8cccfe1ba734fccfe63dc9 (8d16576297d5544e5e8cccfe1ba734fccfe63dc9)

rust-timer · 2022-04-14T22:44:27Z

Queued 8d16576297d5544e5e8cccfe1ba734fccfe63dc9 with parent e7575f9, future comparison URL.

rust-timer · 2022-04-14T23:58:00Z

Finished benchmarking commit (8d16576297d5544e5e8cccfe1ba734fccfe63dc9): comparison url.

Summary:

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	25	30	2	6	27
mean²	0.7%	5.3%	-0.6%	-0.7%	0.6%
max	1.8%	12.9%	-0.6%	-0.9%	1.8%

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 may lead 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

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

…g all information in MIR

oli-obk · 2022-04-25T13:08:09Z

@bors try @rust-timer queue

rust-timer · 2022-04-25T13:08:10Z

Awaiting bors try build completion.

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

bors · 2022-04-25T13:08:17Z

⌛ Trying commit 28274fe with merge 5c48387be095c501f8ff9b8c36f8487870f1acc3...

bors · 2022-04-25T14:35:40Z

☀️ Try build successful - checks-actions
Build commit: 5c48387be095c501f8ff9b8c36f8487870f1acc3 (5c48387be095c501f8ff9b8c36f8487870f1acc3)

rust-timer · 2022-04-25T14:35:42Z

Queued 5c48387be095c501f8ff9b8c36f8487870f1acc3 with parent 756ffb8, future comparison URL.

rust-timer · 2022-04-25T15:58:57Z

Finished benchmarking commit (5c48387be095c501f8ff9b8c36f8487870f1acc3): comparison url.

Summary:

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 😿 relevant regressions found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	9	25	1	0	10
mean²	1.1%	6.0%	-0.7%	N/A	1.0%
max	2.0%	12.4%	-0.7%	N/A	2.0%

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 may lead 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

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

oli-obk · 2022-11-08T13:50:09Z

I don't think these performance regressions are avoidable. I'm abandoning this work

rust-highfive assigned petrochenkov Apr 14, 2022

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

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

rust-highfive assigned wesleywiser and unassigned petrochenkov Apr 14, 2022

JakobDegen reviewed Apr 14, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

oli-obk force-pushed the opaque_aggregates branch from 67ed1a1 to f37e227 Compare April 14, 2022 12:39

oli-obk marked this pull request as ready for review April 14, 2022 20:44

oli-obk force-pushed the opaque_aggregates branch 2 times, most recently from 93ec58d to d316ab5 Compare April 14, 2022 21:15

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

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 14, 2022

oli-obk added 2 commits April 25, 2022 11:46

Add Finalize statement to make deaggregation "reversible" by storin…

75557e4

…g all information in MIR

Move deaggregation before drop elaboration

28274fe

oli-obk force-pushed the opaque_aggregates branch from 41c3ae7 to 28274fe Compare April 25, 2022 13:07

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

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

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 20, 2022

oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2022

oli-obk closed this Nov 8, 2022

Add Finalize statement to make deaggregation "reversible" by storing all information in MIR #96043

Add Finalize statement to make deaggregation "reversible" by storing all information in MIR #96043

Uh oh!

Conversation

oli-obk commented Apr 14, 2022

Uh oh!

rust-highfive commented Apr 14, 2022

Uh oh!

rust-highfive commented Apr 14, 2022

Uh oh!

oli-obk commented Apr 14, 2022

Uh oh!

oli-obk commented Apr 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

oli-obk commented Apr 14, 2022

Uh oh!

rust-timer commented Apr 14, 2022

Uh oh!

bors commented Apr 14, 2022

Uh oh!

bors commented Apr 14, 2022

Uh oh!

rust-timer commented Apr 14, 2022

Uh oh!

rust-timer commented Apr 14, 2022

Footnotes

Uh oh!

oli-obk commented Apr 25, 2022

Uh oh!

rust-timer commented Apr 25, 2022

Uh oh!

bors commented Apr 25, 2022

Uh oh!

bors commented Apr 25, 2022

Uh oh!

rust-timer commented Apr 25, 2022

Uh oh!

rust-timer commented Apr 25, 2022

Footnotes

Uh oh!

oli-obk commented Nov 8, 2022

Uh oh!

Uh oh!

RalfJung May 4, 2022 •

edited

Loading

RalfJung May 5, 2022 •

edited

Loading