Mark inactive enum variants along "otherwise" edge as uninitialized

[ecstatic-morse]

This is a slight improvement on #68528 and #73879. Previously, the interface to the dataflow framework did not allow edge-specific effects along the "otherwise" edge of a SwitchInt terminator. #77242 fixed this. Now we can mark enum variants whose discriminant is handled in an integer-valued SwitchInt edge as definitely uninitialized along the "otherwise" edge. Notably, this applies within an else block attached to an if let expression.

[ecstatic-morse]

@bors try
@rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit e0438129233fbe7e1724b05c52f46bd2f2b89730 with merge d23533d70fe2773aabe056564e0930e7e72b70be...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: d23533d70fe2773aabe056564e0930e7e72b70be (d23533d70fe2773aabe056564e0930e7e72b70be)

[rust-timer]

Queued d23533d70fe2773aabe056564e0930e7e72b70be with parent 867bd42, future comparison URL.

[rust-timer]

Finished benchmarking try commit (d23533d70fe2773aabe056564e0930e7e72b70be): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[ecstatic-morse]

With two exceptions, this is a small win across the board, especially for wall times. However, I'm going to mark this as blocked until #77382 and #74551 are resolved.

[ecstatic-morse]

Thanks to cuviper, we've learned that this class of optimizations is not the root cause of #77382. In a Bayesian sense, I'm now more confident that #74551 is similarly unrelated, although I still don't have the hardware to verify that hypothesis. Given the new data, I'm comfortable getting this on nightly and riding the trains to stable. Most users (including people building rustc) will see a small compile-time speed-up.

[ecstatic-morse]

r? @pnkfelix
cc @oli-obk

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[Dylan-DPC-zz]

@pnkfelix any updates on the review?

[pnkfelix]

Is there a guarantee that the otherwise branch is always invoked last during the iteration performed by edge_effects.apply?

Because, as best as I can tell, it seems like you are relying here on such a guarantee (since you are accumulating all the values in variant_mpis_with_edge during the callback invocations on the non-otherwise edges.

But skimming over the doc comments added in PR #77242, I do not see any such guarantee in the documentation for apply_switch_int_edge_effects.

(And overall, I think the logic here would be easier to understand if the block structure in the code reflected that ordering constraint, even if it requires changes to the dataflow API. I don't know the best way to express it; perhaps by two distinct callbacks instead of just one, where the first callback is invoked for all the edge.value.is_some() cases here, and the second callback for the otherwise case?)

[pnkfelix]

This is a really cool idea.

I will admit I am concerned about whether the code is relying on undocumented guarantees, as noted in a comment on the diff itself.

But that's the main piece of feedback I have at the moment.

[Dylan-DPC-zz]

@ecstatic-morse any updates?

[Dylan-DPC-zz]

Closing this as inactive