Apply effects to `otherwise` edge in dataflow analysis #142707

ashivaram23 · 2025-06-19T07:35:59Z

This allows ElaborateDrops to remove drops when a match wildcard arm covers multiple no-Drop enum variants. It modifies dataflow analysis to update the MaybeUninitializedPlaces and MaybeInitializedPlaces data for a block reached through an otherwise edge.

Fixes #142705.

rustbot · 2025-06-19T07:36:04Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

petrochenkov · 2025-06-19T15:04:01Z

r? compiler

workingjubilee · 2025-06-19T18:35:38Z

hm.
@bors2 try @rust-timer queue

rust-bors · 2025-06-19T18:35:42Z

⌛ Trying commit 5837955 with merge e3d7e41…

To cancel the try build, run the command @bors2 try cancel.

Update `MaybeUninitializedPlaces` and `MaybeInitializedPlaces` for `otherwise`    This allows `ElaborateDrops` to remove drops when a `match` wildcard arm covers multiple no-Drop enum variants. It modifies dataflow analysis to update the `MaybeUninitializedPlaces` and `MaybeInitializedPlaces` data for a block reached through an `otherwise` edge. This appears to fix #142705, but I don't know for sure if it's actually correct (are there cases where this would be wrong and break things?). I also haven't tested that it actually improves compile times, or machine code output outside of the examples in the issue.

rust-bors · 2025-06-19T21:05:46Z

☀️ Try build successful (CI)
Build commit: e3d7e41 (e3d7e41d7e8c513f55398f6e34eb2af7fb7df49f, parent: 8de4c7234dd9b97c9d76b58671343fdbbc9a433e)

rust-timer · 2025-06-20T03:38:26Z

Finished benchmarking commit (e3d7e41): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.6%, 1.5%]	2
Regressions ❌ (secondary)	0.6%	[0.4%, 0.8%]	4
Improvements ✅ (primary)	-0.5%	[-1.2%, -0.2%]	32
Improvements ✅ (secondary)	-0.7%	[-1.3%, -0.2%]	19
All ❌✅ (primary)	-0.4%	[-1.2%, 1.5%]	34

Max RSS (memory usage)

Results (primary -0.2%, secondary 2.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.5%	[2.3%, 4.7%]	3
Regressions ❌ (secondary)	2.8%	[2.2%, 3.5%]	2
Improvements ✅ (primary)	-2.4%	[-3.6%, -0.5%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-3.6%, 4.7%]	8

Cycles

Results (primary 0.9%, secondary -5.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.7%	[2.6%, 2.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.2%, -0.8%]	2
Improvements ✅ (secondary)	-5.0%	[-5.0%, -5.0%]	1
All ❌✅ (primary)	0.9%	[-1.2%, 2.9%]	4

Binary size

Results (primary -0.1%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	7
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	7
Improvements ✅ (primary)	-0.1%	[-0.5%, -0.0%]	64
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	50
All ❌✅ (primary)	-0.1%	[-0.5%, 0.2%]	71

Bootstrap: 691.904s -> 692.572s (0.10%)
Artifact size: 372.02 MiB -> 371.98 MiB (-0.01%)

ashivaram23 · 2025-06-20T06:01:48Z

Hmm that's unfortunate. It might help to avoid paying the cost of the extra clone and checks when it won't make a difference, like in the trivial otherwise targets when there's no wildcard. Maybe the otherwise could be handled in a separate pass after collecting variants to kill/add in a SmallVec, in case that helps somehow.

What's more concerning is runtime benchmarks being worse. There are only two, and I don't know how noisy they are, but 8% wall time increase for css-parse-fb is pretty bad. There are also some compile time benchmarks whose graphs show significant increases in wall time spent in LLVM. Does this just not play well with later optimizations?

rustbot · 2025-06-21T01:04:38Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

ashivaram23 · 2025-06-21T01:15:12Z

The last commit makes it so that there's an option in the MaybeUninitializedPlaces and MaybeInitializedPlaces builders for whether or not to update the otherwise block's lattice element, and it's set to only do so for MaybeInitializedPlaces in RemoveUninitDrops and ElaborateDrops. That should be enough to clean up some drops while hopefully being faster and not messing with other passes in ways that cause runtime slowdowns.

workingjubilee · 2025-06-21T01:42:24Z

the runtime benchmarks are unfortunately very noisy

fee1-dead · 2025-06-22T11:36:05Z

compiler/rustc_mir_dataflow/src/drop_flag_effects.rs

    move_data: &MoveData<'tcx>,
    enum_place: mir::Place<'tcx>,
    active_variant: VariantIdx,
    mut handle_inactive_variant: impl FnMut(MovePathIndex),
+    mut handle_active_variant: Option<impl FnMut(MovePathIndex)>,


I wonder if it will be better, perf-wise to make this not take an option but just use a no-op for callers that do not want to handle it.

That's definitely possible, since I think in that case on_all_children_bits could be monomorphized into an empty function. I made the change and it does reduce stage2 librustc_driver.so size a bit.

fee1-dead · 2025-06-22T11:37:27Z

this part of MIR is a little above my level of confidence for review, so

r? compiler

lcnr · 2025-06-23T12:01:54Z

r? wg-mir-opt

rustbot · 2025-06-29T11:22:48Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

ashivaram23 · 2025-07-07T09:38:29Z

Sorry it took me a while to get to this! I added a test in otherwise_drops.rs.

As for your suggestion, I think it makes sense. It's more readable and while it does involve an additional allocation and another pass through the variants and move paths, it also saves a clone of the dataflow state, and the non-otherwise targets do the same quadratic loops anyway (could only be avoided if there was a guarantee that move paths are in the same order as discriminants).

I can implement the ActiveVariants enum suggestion as is, but I was thinking of combining it with a few other changes that may or may not be a good idea, and would like to hear your thoughts.

My idea was that it could be nicer to collect the VariantIdxs of each switch target while creating MaybePlacesSwitchIntData so they could be passed directly to on_all_inactive_variants without a new allocation, and the non-otherwise targets won't have to skip over variants with next_discr every time either. Nobody else uses MaybePlacesSwitchIntData and its main purpose seems to be mapping switch target u128 values to VariantIdx, so we can avoid the extra work and indirection with something like this:

pub struct MaybePlacesSwitchIntData<'tcx> {
    enum_place: mir::Place<'tcx>,
    
    // only the targets in the SwitchInt, not all discriminants
    targets: Vec<(VariantIdx, mir::BasicBlock)>,

    // discriminants: Vec<(VariantIdx, Discr<'tcx>)>,
    // index: usize,
}

Then SwitchTargetValue could store VariantIdx instead of u128, so the whole next_discr process and index state tracking would be unnecessary, and the otherwise pass could use this filtered targets list that contains the VariantIdxs without needing to allocate a new Vec for it.

This would probably require removing the call to BasicBlocks::switch_sources in backward apply_effects_in_block because it's tough to make switch_sources get VariantIdxs. Setting aside how all of the backward code is currently unused, I think that could make it more efficient. Since get_switch_int_data is being called on each predecessor* anyway, it's not necessary to loop through targets returned by switch_sources (which goes through every block in the CFG on the first call, though that gets cached). The only benefit of doing so at the moment is avoiding a check to filter for the correct successor, but the point of this idea is for get_switch_int_data to filter everything upfront.

* To me it seems like it should be called on the block's predecessor, but it's currently being passed the block itself. Again it's unused so it doesn't matter, but I wonder if this was actually a typo. @nnethercote because this line is from #133328.

dianqk

Maybe, but I think it should be an independent PR.

dianqk · 2025-07-07T12:27:22Z

tests/mir-opt/otherwise_drops.rs

+
+// Ensures there are no drops for the wildcard match arm.
+
+// EMIT_MIR otherwise_drops.result_ok.ElaborateDrops.after.mir


Why not EMIT_MIR otherwise_drops.result_ok.ElaborateDrops.diff

When I run ./x test --bless with diff output, the first runs generate a slightly different "before" than the later runs, which means running ./x test immediately afterwards fails. There's one line that starts out as unwind: continue in the first platform tests and then becomes unwind: bb9 later.

Perhaps you want to add //@ compile-flags: -Cpanic=abort or // EMIT_MIR_FOR_EACH_PANIC_STRATEGY. I prefer the first one here.

dianqk · 2025-07-07T12:29:56Z

tests/mir-opt/otherwise_drops.rs

@@ -0,0 +1,16 @@
+// skip-filecheck


Please add what you want to check, such as // CHECK-NOT: drop(.

dianqk · 2025-07-07T12:36:54Z

BTW, the PR title could probably be something like: "Applying effects to otherwise in dataflow"

ashivaram23 · 2025-07-08T06:34:18Z

@rustbot ready

ashivaram23 · 2025-07-08T06:36:07Z

In the last commit I fixed the test file and incorporated the suggestions. I used a SmallVec since that's what SwitchTargets and other structs do, and I switched it to InactiveVariants for the double negative.

tmiasko · 2025-07-08T06:38:25Z

To me it seems like it should be called on the block's predecessor, but it's currently being passed the block itself.

Yes, it should have been called on the predecessor which contains the switch.

I haven't followed all the discussion, but if removing unused switch int handling in the backward direction makes anything easier that seems perfectly fine.

dianqk

Please squash your commits into one and update the PR description, remembering to use https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword.

dianqk · 2025-07-08T11:28:30Z

tests/mir-opt/otherwise_drops.rs

+
+// Ensures there are no drops for the wildcard match arm.
+
+// EMIT_MIR otherwise_drops.result_ok.ElaborateDrops.after.mir


Perhaps you want to add //@ compile-flags: -Cpanic=abort or // EMIT_MIR_FOR_EACH_PANIC_STRATEGY. I prefer the first one here.

dianqk · 2025-07-08T11:33:30Z

tests/mir-opt/otherwise_drops.rs

+fn result_ok(result: Result<String, ()>) -> Option<String> {
+    // CHECK-LABEL: fn result_ok(
+    // CHECK-NOT: drop
+    // CHECK: return


Suggested change

// CHECK: return

The return statement isn't the final line of this function.

dianqk · 2025-07-08T11:34:26Z

tests/mir-opt/otherwise_drops.rs

+fn main() {
+    result_ok(Err(()));
+}


Suggested change

fn main() {

result_ok(Err(()));

}

Removing this function makes file check easier.

dianqk · 2025-07-08T11:54:46Z

@bors2 try @rust-timer queue

rust-bors · 2025-07-08T11:54:50Z

⌛ Trying commit 881c62f with merge 237f435…

To cancel the try build, run the command @bors2 try cancel.

Apply effects to `otherwise` edge in dataflow analysis    This allows `ElaborateDrops` to remove drops when a `match` wildcard arm covers multiple no-Drop enum variants. It modifies dataflow analysis to update the `MaybeUninitializedPlaces` and `MaybeInitializedPlaces` data for a block reached through an `otherwise` edge. This appears to fix #142705, but I don't know for sure if it's actually correct (are there cases where this would be wrong and break things?). I also haven't tested that it actually improves compile times, or machine code output outside of the examples in the issue.

rust-bors · 2025-07-08T14:10:57Z

☀️ Try build successful (CI)
Build commit: 237f435 (237f435668e899ed46d943ff30ee6f097b6b03fb, parent: 45b80ac21a454d343833aad763ef604510c88375)

rust-timer · 2025-07-08T16:16:01Z

Finished benchmarking commit (237f435): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	0.6%	[0.2%, 0.9%]	13
Improvements ✅ (primary)	-0.4%	[-1.1%, -0.1%]	15
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.1%]	7
All ❌✅ (primary)	-0.3%	[-1.1%, 0.7%]	16

Max RSS (memory usage)

Results (primary 2.7%, secondary 3.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	4.8%	[3.5%, 6.2%]	2
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-1.7%	[-1.7%, -1.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.7%	[-1.7%, 6.2%]	3

Cycles

Results (primary 1.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.4%, 2.5%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[-2.0%, 2.5%]	3

Binary size

Results (primary -0.1%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.0%, 1.4%]	10
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	4
Improvements ✅ (primary)	-0.1%	[-1.5%, -0.0%]	63
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.0%]	51
All ❌✅ (primary)	-0.1%	[-1.5%, 1.4%]	73

Bootstrap: 467.247s -> 463.296s (-0.85%)
Artifact size: 372.36 MiB -> 372.30 MiB (-0.02%)

ashivaram23 · 2025-07-08T23:10:00Z

Squashed and force pushed, the only changes since the last commit are addressing the test file review comments.

Regarding the perf run results, there are 8 cases this could affect:

MaybeInitializedPlaces in MIR type check liveness analysis
MaybeUninitializedPlaces in MIR borrow check
MaybeInitializedPlaces in SanityCheck
MaybeUninitializedPlaces in SanityCheck
MaybeInitializedPlaces in lint_tail_expr_drop_order
MaybeInitializedPlaces in RemoveUninitDrops
MaybeInitializedPlaces in ElaborateDrops
MaybeUninitializedPlaces in ElaborateDrops

The first perf run handled otherwise edges everywhere, the second did it only for 6 and 7, and the third did it for 6-8.

The first has the strongest results, with a lot of improvements and a few regressions. (Also a runtime test regression, which seems to follow a step function where it fluctuates between either exactly 0 or exactly 1.9% relative to the baseline over the past 30 days and went back to 1.9% here, not sure what that means)

The second still shows an overall improvement in compile times but applies to much fewer tests. It also has a different runtime regression with the same binary/step pattern.

The third has more regressions and fewer improvements, and an overall regression in compile times for all benchmarks (mostly due to the secondary benchmarks especially tt-muncher, it's a small overall improvement for the primary ones only). But there are no runtime test regressions and bootstrap times are four seconds faster.

Should I just enable the change for all 8 cases again? Or make it as limited as possible (number 7 alone is enough to fix the wildcard problem), or try other combinations?

dianqk · 2025-07-09T00:51:08Z

These can be subsequent PRs. Thanks!
@bors r+

bors · 2025-07-09T00:51:10Z

📌 Commit c7ef03a has been approved by dianqk

It is now in the queue for this repository.

rustbot assigned petrochenkov Jun 19, 2025

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 Jun 19, 2025

This comment has been minimized.

Sign in to view

rustbot assigned fee1-dead and unassigned petrochenkov Jun 19, 2025

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 Jun 19, 2025

This comment has been minimized.

Sign in to view

ashivaram23 force-pushed the drop_wildcard branch from 8d837ad to 10e1a9d Compare June 20, 2025 00:45

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 20, 2025

fee1-dead reviewed Jun 22, 2025

View reviewed changes

rustbot assigned lcnr and unassigned fee1-dead Jun 22, 2025

rustbot assigned dianqk and unassigned lcnr Jun 23, 2025

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 29, 2025

This comment has been minimized.

Sign in to view

dianqk reviewed Jul 7, 2025

View reviewed changes

ashivaram23 changed the title ~~Update MaybeUninitializedPlaces and MaybeInitializedPlaces for otherwise~~ Apply effects to otherwise edge in dataflow analysis Jul 8, 2025

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

dianqk approved these changes Jul 8, 2025

View reviewed changes

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 Jul 8, 2025

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 Jul 8, 2025

ashivaram23 force-pushed the drop_wildcard branch from 881c62f to d9dd6ee Compare July 8, 2025 23:04

Apply effects to otherwise edge in dataflow analysis

c7ef03a

ashivaram23 force-pushed the drop_wildcard branch from d9dd6ee to c7ef03a Compare July 8, 2025 23:15

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 Jul 9, 2025


		// Ensures there are no drops for the wildcard match arm.

		// EMIT_MIR otherwise_drops.result_ok.ElaborateDrops.after.mir

Apply effects to otherwise edge in dataflow analysis #142707

Are you sure you want to change the base?

Apply effects to otherwise edge in dataflow analysis #142707

Uh oh!

Conversation

ashivaram23 commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 19, 2025

Uh oh!

This comment has been minimized.

petrochenkov commented Jun 19, 2025

Uh oh!

workingjubilee commented Jun 19, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Jun 19, 2025

Uh oh!

rust-bors bot commented Jun 19, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Jun 20, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Uh oh!

ashivaram23 commented Jun 20, 2025

Uh oh!

rustbot commented Jun 21, 2025

Uh oh!

ashivaram23 commented Jun 21, 2025

Uh oh!

workingjubilee commented Jun 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fee1-dead commented Jun 22, 2025

Uh oh!

lcnr commented Jun 23, 2025

Uh oh!

rustbot commented Jun 29, 2025

Uh oh!

ashivaram23 commented Jul 7, 2025

Uh oh!

This comment has been minimized.

dianqk left a comment

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!

dianqk commented Jul 7, 2025

Uh oh!

ashivaram23 commented Jul 8, 2025

Uh oh!

ashivaram23 commented Jul 8, 2025

Uh oh!

tmiasko commented Jul 8, 2025

Uh oh!

dianqk left a comment

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!

dianqk commented Jul 8, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Jul 8, 2025

Uh oh!

rust-bors bot commented Jul 8, 2025

Uh oh!

Apply effects to `otherwise` edge in dataflow analysis #142707

Apply effects to `otherwise` edge in dataflow analysis #142707

ashivaram23 commented Jun 19, 2025 •

edited

Loading