Fix Dest Prop #96451

JakobDegen · 2022-04-26T20:22:38Z

This was not originally a total re-write of the pass but is has gradually turned into one. Notable changes:

Significant improvements to documentation all around. The top of the file has been extended with a more precise argument for soundness. The code should be fairly readable, and I've done my best to add useful comments wherever possible. I would very much like for the bus factor to not be one on this code.
Improved handling of conflicts that are not visible in normal dataflow. This was the cause of Incorrectly unified locals at -Zmir-opt-level=2 #79191. Handling this correctly requires us to make decision about the semantics and specifically evaluation order of basically all MIR constructs (see specifically Semantics of MIR assignments, around aliasing, ordering, and primitives. #68364 Semantics of MIR function calls #71117. The way this is implemented is based on my preferred resolution to these questions around the semantics of assignment statements.
Some re-architecting to improve performance. More details below.
Possible future improvements to this optimization are documented, and the code is written with the needs of those improvements in mind. The hope is that adding support for more precise analyses will not require a full re-write of this opt, but just localized changes.

Regarding Performance

The previous approach had some performance issues; letting l be the number of locals and s be the number of statements/terminators, the runtime of the pass was O(l^2 * s), both in theory and in practice. This version is smarter about not calculating unnecessary things and doing more caching. Our runtime is now dominated by one invocation of MaybeLiveLocals for each "round," and the number of rounds is less than 5 in over 90% of cases. This means it's linear-ish in practice.

r? @oli-obk who reviewed the last version of this, but review from anyone else would be more than welcome

JakobDegen · 2022-04-26T20:24:26Z

@bors try @rust-timer queue

rust-timer · 2022-04-26T20:24:27Z

Awaiting bors try build completion.

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

bors · 2022-04-26T20:24:34Z

⌛ Trying commit ac672ab9c0f14ea470820c5b9e1ac24d5cd3b536 with merge 86d0ce429f15243ad8abb713ac10f80134ca0832...

JakobDegen · 2022-04-26T20:44:07Z

Hmm, didn't hit that failure locally. Will try and debug tomorrow

bors · 2022-04-26T21:32:16Z

💔 Test failed - checks-actions

JakobDegen · 2022-04-28T18:46:49Z

Agh. This might have to wait until next week, my laptop can't handle running tests and that might make things difficult

JakobDegen · 2022-04-29T15:12:57Z

I actually did get a chance to take a look at this, and I think what's going on is this: The test failure was not indicative of a bug, but rather was "expected" in that dest prop eliminated the local whose mutability changed, and so now the optimized MIR no longer differed between the revisions. I confirmed by looking at the PreCodegen.after dump of both revisions, once with dest prop on and once with it off, and did indeed see a difference in the local declarations as I would have expected.

Fixing this by turning off optimizations seems like a principled approach, since "optimized MIR differs due to a trivial change in the source code" does not seem like something we actually want to be true.

oli-obk · 2022-04-29T15:22:01Z

does not seem like something we actually want to be true.

but... without optimizations this is now true afaict? You could leave optimizations on and remove the dirty flag for optimized_mir, there are other queries that are still dirty, so it's obvious that something is still changing.

JakobDegen · 2022-04-29T15:29:42Z

Eh, yeah, when I say "optimized MIR" I mean "optimized MIR with optimizations on." Removing the dirty flag is also an option, but doesn't that test less? After all, ensuring that the relevant revision causes changes in MIR if we don't optimize still seems like something we want

oli-obk · 2022-04-29T15:38:15Z

yea... there's merit for both... 🤷 let's do yours for now, it's more representative of what we had

cjgillot · 2022-04-29T21:18:49Z

Removing the dirty flag in the incremental test is the right thing to do here. This allows to track the evolution of the incremental behaviour. (And actually, the less "dirty" flags there are in those files, the better).

JakobDegen · 2022-04-29T21:34:00Z

Hmm, ok. Will change that next time I push

JakobDegen · 2022-04-30T12:24:01Z

Oh, wait, can try this again now...

@bors try @rust-timer queue

rust-timer · 2022-04-30T12:24:03Z

Awaiting bors try build completion.

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

bors · 2022-04-30T12:24:09Z

⌛ Trying commit af83957d606cf4a6740b7cc30e7e29edb1746851 with merge 301aa4b1a05417c4803bbf04f9bbb4cbec740f61...

rust-timer · 2022-10-31T09:47:48Z

Queued 28afc0618fc643fded6958846b8f8a06d5ae81f4 with parent 4596f4f, future comparison URL.

rust-timer · 2022-10-31T11:06:33Z

Finished benchmarking commit (28afc0618fc643fded6958846b8f8a06d5ae81f4): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

JakobDegen · 2022-10-31T17:30:07Z

I can comment the pass out for now if you want to, I did measure non-negligible impact in some cases once this is turned on though

bors · 2022-11-15T13:21:00Z

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

tmiasko · 2022-11-15T14:24:54Z

Removing unused local declarations immediately after dead store elimination sounds good. Just wanted to check the potential compile time impact of pull request in the form it would actually land.

r=me when rebased.
@bors delegate+

bors · 2022-11-15T14:24:56Z

✌️ @JakobDegen can now approve this pull request

This fixes a number of correctness issues from the previous version. Additionally, we use a new strategy which has much better performance charactersitics and also finds more opportunities to apply the optimization.

JakobDegen · 2022-11-27T03:04:49Z

@bors r=tmiasko

bors · 2022-11-27T03:04:51Z

📌 Commit 245c607 has been approved by tmiasko

It is now in the queue for this repository.

bors · 2022-11-27T04:09:57Z

⌛ Testing commit 245c607 with merge 0e9eee6...

bors · 2022-11-27T07:11:56Z

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing 0e9eee6 to master...

rust-timer · 2022-11-27T08:31:06Z

Finished benchmarking commit (0e9eee6): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)	2.7%	[2.7%, 2.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.7%	[2.7%, 2.7%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

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

rust-highfive assigned oli-obk Apr 26, 2022

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

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

oli-obk added the A-mir-opt Area: MIR optimizations label Apr 26, 2022

bors 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 Apr 26, 2022

This comment has been minimized.

Sign in to view

oli-obk mentioned this pull request Apr 28, 2022

WIP: Avoid storing captured upvars in generators twice if possible #89213

Closed

JakobDegen force-pushed the dest-prop branch from ac672ab to 062864f Compare April 28, 2022 18:25

This comment has been minimized.

Sign in to view

JakobDegen force-pushed the dest-prop branch from c753877 to af83957 Compare April 29, 2022 21:09

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Oct 31, 2022

JakobDegen force-pushed the dest-prop branch from 3685ce7 to 98c884a Compare November 13, 2022 06:20

tmiasko removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 23, 2022

Rewrite dest prop.

245c607

This fixes a number of correctness issues from the previous version. Additionally, we use a new strategy which has much better performance charactersitics and also finds more opportunities to apply the optimization.

JakobDegen force-pushed the dest-prop branch from 98c884a to 245c607 Compare November 27, 2022 02:05

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 27, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 27, 2022

bors merged commit 0e9eee6 into rust-lang:master Nov 27, 2022

rustbot added this to the 1.67.0 milestone Nov 27, 2022

This was referenced Nov 27, 2022

Introduce a no-op PlaceMention statement for let _ =. #102256

Merged

Remove identity_future indirection #104833

Merged

Evaluate place expression in PlaceMention #104844

Merged

Alexendoo mentioned this pull request Nov 28, 2022

portable simd: -Zmir-opt-level=3 causes LLVM ERROR #98016

Closed

JakobDegen mentioned this pull request Dec 2, 2022

Convert Operand::Move to Operand::Copy #105190

Closed

tmiasko mentioned this pull request Dec 5, 2022

Incorrectly unified locals at -Zmir-opt-level=2 #79191

Closed

JakobDegen deleted the dest-prop branch December 6, 2022 11:58

JakobDegen mentioned this pull request Dec 13, 2022

Do one round of DestProp at mir-opt-level=2 #105577

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Dest Prop #96451

Fix Dest Prop #96451

JakobDegen commented Apr 26, 2022 •

edited

Loading

JakobDegen commented Apr 26, 2022

rust-timer commented Apr 26, 2022

bors commented Apr 26, 2022

JakobDegen commented Apr 26, 2022

bors commented Apr 26, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

JakobDegen commented Apr 28, 2022

JakobDegen commented Apr 29, 2022

oli-obk commented Apr 29, 2022

JakobDegen commented Apr 29, 2022 •

edited

Loading

oli-obk commented Apr 29, 2022

This comment has been minimized.

cjgillot commented Apr 29, 2022

JakobDegen commented Apr 29, 2022 •

edited

Loading

JakobDegen commented Apr 30, 2022

rust-timer commented Apr 30, 2022

bors commented Apr 30, 2022

rust-timer commented Oct 31, 2022

rust-timer commented Oct 31, 2022

JakobDegen commented Oct 31, 2022

bors commented Nov 15, 2022

tmiasko commented Nov 15, 2022

bors commented Nov 15, 2022

JakobDegen commented Nov 27, 2022

bors commented Nov 27, 2022

bors commented Nov 27, 2022

bors commented Nov 27, 2022

rust-timer commented Nov 27, 2022

Fix Dest Prop #96451

Fix Dest Prop #96451

Conversation

JakobDegen commented Apr 26, 2022 • edited Loading

Regarding Performance

JakobDegen commented Apr 26, 2022

rust-timer commented Apr 26, 2022

bors commented Apr 26, 2022

JakobDegen commented Apr 26, 2022

bors commented Apr 26, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

JakobDegen commented Apr 28, 2022

JakobDegen commented Apr 29, 2022

oli-obk commented Apr 29, 2022

JakobDegen commented Apr 29, 2022 • edited Loading

oli-obk commented Apr 29, 2022

This comment has been minimized.

cjgillot commented Apr 29, 2022

JakobDegen commented Apr 29, 2022 • edited Loading

JakobDegen commented Apr 30, 2022

rust-timer commented Apr 30, 2022

bors commented Apr 30, 2022

rust-timer commented Oct 31, 2022

rust-timer commented Oct 31, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

JakobDegen commented Oct 31, 2022

bors commented Nov 15, 2022

tmiasko commented Nov 15, 2022

bors commented Nov 15, 2022

JakobDegen commented Nov 27, 2022

bors commented Nov 27, 2022

bors commented Nov 27, 2022

bors commented Nov 27, 2022

rust-timer commented Nov 27, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

JakobDegen commented Apr 26, 2022 •

edited

Loading

JakobDegen commented Apr 29, 2022 •

edited

Loading

JakobDegen commented Apr 29, 2022 •

edited

Loading