Only run MaybeInitializedPlaces dataflow once to elaborate drops #111555

cjgillot · 2023-05-14T10:06:49Z

This pass allows forward dataflow analyses to modify the CFG depending on the dataflow state. This possibility is used for the MaybeInitializedPlace analysis in drop elaboration, to skip the dataflow effect of dead unwinds without having to compute dataflow twice.

cjgillot · 2023-05-14T10:37:07Z

@bors try @rust-timer queue

bors · 2023-05-14T10:37:16Z

⌛ Trying commit 6185b06e28ff0e0699b0b4c13f8a1375371cc13e with merge 4436f122442131f57e5dcd6b62dd883bfd5e8204...

bors · 2023-05-14T12:46:15Z

☀️ Try build successful - checks-actions
Build commit: 4436f122442131f57e5dcd6b62dd883bfd5e8204 (4436f122442131f57e5dcd6b62dd883bfd5e8204)

rust-timer · 2023-05-14T13:59:46Z

Finished benchmarking commit (4436f122442131f57e5dcd6b62dd883bfd5e8204): comparison URL.

Overall result: ✅ 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-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)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.2%, -0.3%]	30
Improvements ✅ (secondary)	-0.9%	[-1.3%, -0.2%]	29
All ❌✅ (primary)	-0.7%	[-1.2%, -0.3%]	30

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)	1.9%	[1.4%, 2.2%]	3
Improvements ✅ (primary)	-4.6%	[-5.2%, -4.3%]	3
Improvements ✅ (secondary)	-8.8%	[-13.2%, -2.2%]	3
All ❌✅ (primary)	-4.6%	[-5.2%, -4.3%]	3

Cycles

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

Binary size

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

Bootstrap: 660.613s -> 659.115s (-0.23%)

rustbot · 2023-05-14T14:09:32Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

cjgillot · 2023-05-14T14:09:35Z

r? compiler

compiler/rustc_mir_dataflow/src/framework/lattice.rs

compiler/rustc_middle/src/mir/terminator.rs

compiler/rustc_mir_dataflow/src/impls/liveness.rs

tmiasko · 2023-05-16T07:56:37Z

compiler/rustc_mir_transform/src/elaborate_drops.rs

+            let mut reachable = BitSet::new_empty(body.basic_blocks.len());
+            for bb in body.basic_blocks.indices() {
+                if inits.results().entry_set_for_block(bb).is_reachable() {
+                    reachable.insert(bb);
+                }
+            }


It looks like reachable and all uses that follow could be removed now. Do you think it is still useful? As I understand, this boils down to difference between unreachable unelaborated drop, and unreachable elaborated drop that doesn't drop anything. So it might be simpler to just remove this.

Although, in any case, to minimize the diff I would leave that for a potential followup.

This is still used to avoid a seek into an unreachable block. The dataflow framework ICEs in certain conditions in those cases.

Initially there are no structurally unreachable blocks (in a sense of traversal::reachable), so we can execute the dataflow engine without running into ICE. In the new approach, the subsequent execution of dataflow engine uses the same MIR body, so we shouldn't have any structurally unreachable blocks still.

compiler/rustc_mir_dataflow/src/framework/direction.rs

bors · 2023-05-27T16:01:21Z

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

compiler/rustc_mir_transform/src/dataflow_const_prop.rs

bors · 2023-06-27T21:36:02Z

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

bors · 2023-07-03T05:47:21Z

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

tmiasko · 2023-08-16T22:08:36Z

@bors r+

bors · 2023-08-16T22:08:38Z

📌 Commit 5b2524e has been approved by tmiasko

It is now in the queue for this repository.

bors · 2023-08-16T23:21:20Z

⌛ Testing commit 5b2524e with merge f3b4c67...

bors · 2023-08-17T01:04:31Z

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

rust-timer · 2023-08-17T03:19:01Z

Finished benchmarking commit (f3b4c67): comparison URL.

Overall result: ✅ 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)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-0.8%	[-1.3%, -0.3%]	27
Improvements ✅ (secondary)	-0.7%	[-1.1%, -0.4%]	32
All ❌✅ (primary)	-0.8%	[-1.3%, -0.3%]	27

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

Binary size

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

Bootstrap: 635.21s -> 632.126s (-0.49%)
Artifact size: 346.72 MiB -> 346.51 MiB (-0.06%)

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 14, 2023

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 14, 2023

cjgillot marked this pull request as ready for review May 14, 2023 14:09

rustbot assigned wesleywiser May 14, 2023

tmiasko reviewed May 14, 2023

View reviewed changes

compiler/rustc_mir_dataflow/src/framework/lattice.rs Outdated Show resolved Hide resolved

cjgillot force-pushed the elaborate-drops branch from 6185b06 to 140a34b Compare May 19, 2023 17:41

tmiasko reviewed May 22, 2023

View reviewed changes

cjgillot force-pushed the elaborate-drops branch 2 times, most recently from 261b20e to 168e725 Compare May 28, 2023 06:49

tmiasko reviewed May 28, 2023

View reviewed changes

compiler/rustc_mir_transform/src/dataflow_const_prop.rs Outdated Show resolved Hide resolved

cjgillot force-pushed the elaborate-drops branch from 168e725 to 11a9082 Compare May 29, 2023 10:30

This comment has been minimized.

Sign in to view

cjgillot force-pushed the elaborate-drops branch from 11a9082 to a7fafc8 Compare May 29, 2023 11:13

cjgillot force-pushed the elaborate-drops branch from a7fafc8 to 6930099 Compare June 24, 2023 08:41

cjgillot force-pushed the elaborate-drops branch from 6930099 to cd2d25e Compare July 2, 2023 13:38

cjgillot force-pushed the elaborate-drops branch from cd2d25e to aedb1c2 Compare July 3, 2023 17:03

cjgillot added 12 commits August 16, 2023 18:12

Move domain_size to GenKillAnalysis.

934a99e

Introduce MaybeUnreachable.

32711b2

Allow apply_terminator_effect to customize edges.

5173d85

Only run MaybeInitializedPlaces once for drop elaboration.

3acfa09

Use TerminatorEdge for dataflow-const-prop.

f19cd3f

Rename MaybeUnreachable.

6cf15d4

Make TerminatorEdge plural.

388f6a6

Rename YieldResumeEffect.

aa697f5

Specify that method only applies statement effects.

7ded340

Only evaluate yield place after resume in liveness.

e9990ce

Make dataflow const-prop handle_switch_int monotonic.

b8fed2f

Update doc comment.

94c5ea3

cjgillot force-pushed the elaborate-drops branch from 84f47e9 to 9f2da27 Compare August 16, 2023 18:59

This comment has been minimized.

Sign in to view

cjgillot added 2 commits August 16, 2023 19:40

Use Terminator::edges for backward analysis too.

f5e4eb9

Do not pre-compute reachable blocks.

5b2524e

cjgillot force-pushed the elaborate-drops branch from 9f2da27 to 5b2524e Compare August 16, 2023 19:40

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 Aug 16, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 17, 2023

bors merged commit f3b4c67 into rust-lang:master Aug 17, 2023

rustbot added this to the 1.73.0 milestone Aug 17, 2023

This was referenced Aug 17, 2023

Compute NLL loan scopes using the polonius model #113218

Merged

Allow a MIR analysis to perform the state join directly #114900

Open

cjgillot deleted the elaborate-drops branch August 18, 2023 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only run MaybeInitializedPlaces dataflow once to elaborate drops #111555

Only run MaybeInitializedPlaces dataflow once to elaborate drops #111555

cjgillot commented May 14, 2023 •

edited

Loading

cjgillot commented May 14, 2023

This comment has been minimized.

bors commented May 14, 2023

bors commented May 14, 2023

This comment has been minimized.

rust-timer commented May 14, 2023

rustbot commented May 14, 2023

cjgillot commented May 14, 2023

tmiasko May 16, 2023

cjgillot Aug 3, 2023

tmiasko Aug 15, 2023

bors commented May 27, 2023

This comment has been minimized.

bors commented Jun 27, 2023

bors commented Jul 3, 2023

This comment has been minimized.

tmiasko commented Aug 16, 2023

bors commented Aug 16, 2023

bors commented Aug 16, 2023

bors commented Aug 17, 2023

rust-timer commented Aug 17, 2023

Only run MaybeInitializedPlaces dataflow once to elaborate drops #111555

Only run MaybeInitializedPlaces dataflow once to elaborate drops #111555

Conversation

cjgillot commented May 14, 2023 • edited Loading

cjgillot commented May 14, 2023

This comment has been minimized.

bors commented May 14, 2023

bors commented May 14, 2023

This comment has been minimized.

rust-timer commented May 14, 2023

Overall result: ✅ improvements - no action needed

rustbot commented May 14, 2023

cjgillot commented May 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 27, 2023

This comment has been minimized.

bors commented Jun 27, 2023

bors commented Jul 3, 2023

This comment has been minimized.

tmiasko commented Aug 16, 2023

bors commented Aug 16, 2023

bors commented Aug 16, 2023

bors commented Aug 17, 2023

rust-timer commented Aug 17, 2023

Overall result: ✅ improvements - no action needed

cjgillot commented May 14, 2023 •

edited

Loading