Still more `rustc_mir_dataflow` cleanups #132062

nnethercote · 2024-10-23T10:22:30Z

A sequel to #118203, #118230, #118638, and #131481.

I'm pleased with this PR. It cleans up a few things that have been bugging me for a while. It took quite some effort to find these improvements.

r? @cjgillot

This requires a `clone` call in `check_for_move`, but makes `MaybeRequiresStorage` an immutable analysis, which enables the next commit.

rustbot · 2024-10-23T10:22:50Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Much nicer.

nnethercote · 2024-10-23T23:38:50Z

@bors try @rust-timer queue

…aflow-cleanups, r=<try> Still more `rustc_mir_dataflow` cleanups A sequel to rust-lang#118203, rust-lang#118230, rust-lang#118638, and rust-lang#131481. I'm pleased with this PR. It cleans up a few things that have been bugging me for a while. It took quite some effort to find these improvements. r? `@cjgillot`

bors · 2024-10-23T23:40:03Z

⌛ Trying commit 9e735ca with merge 1c188eb...

bors · 2024-10-24T01:33:11Z

☀️ Try build successful - checks-actions
Build commit: 1c188eb (1c188eb1f522be2697a49819f69932c2a96181ee)

Because most of them are. The one exception is `ConstAnalysis`, which gains a `RefCell` to provide interior mutability. This is a bit annoying, but I think it's worth inconveniencing the one analysis that requires mutability in exchange for making the interface simpler for all the other analyses.

Because it's a `ResultsCursor`, not a `Results`.

`Formatter` currently has a `RefCell<Option<Results>>` field. This is so the `Results` can be temporarily taken and put into a `ResultsCursor` that is used by `BlockFormatter`, and then put back, which is messy. This commit changes `Formatter` to have a `RefCell<ResultsCursor>` and `BlockFormatter` to have a `&mut ResultsCursor`, which greatly simplifies the code at the `Formatter`/`BlockFormatter` interaction point in `Formatter::node_label`. The commit also: - documents the reason for the `RefCell`; - adds a `Formatter::body` method, replacing the `Formatter::body` field.

Instead of mutating an empy label. Because it's conceptually simpler.

This is a standard pattern: ``` MyAnalysis.into_engine(tcx, body).iterate_to_fixpoint() ``` In fact, `into_engine` and `iterate_to_fixpoint` are always called in pairs, but sometimes with a builder-style `pass_name` calls between them. But a builder-style interface is overkill here. This has been bugging me a for a while. This commit: - Merges `Engine::new` and `Engine::iterate_to_fixpoint`. This removes the need for `Engine` to have fields, leaving it as a trivial type that the next commit will remove. - Renames `Analysis::into_engine` as `Analysis::iterate_to_fixpoint`, gives it an extra argument for the optional pass name, and makes it call `Engine::iterate_to_fixpoint` instead of `Engine::new`. This turns the pattern from above into this: ``` MyAnalysis.iterate_to_fixpoint(tcx, body, None) ``` which is shorter at every call site, and there's less plumbing required to support it.

It's no longer needed. `Engine::iterate_to_fixpoint` can be inlined into `Analysis::iterate_to_fixpoint` and removed. The commit also renames `engine.rs` as `results.rs`.

`ResultsCursor` currently owns its `Results`. But sometimes the `Results` is needed again afterwards. So there is `ResultsCursor::into_results` for extracting the `Results`, which leads to some awkwardness. This commit adds `ResultsHandle`, a `Cow`-like type that can either borrow or own a a `Results`. `ResultsCursor` now uses it. This is good because some `ResultsCursor`s really want to own their `Results`, while others just want to borrow it. We end with with a few more lines of code, but get some nice cleanups. - `ResultsCursor::into_results` is removed. - `Formatter::into_results` is removed. - `write_graphviz_results` now just borrows a `Results`, instead of the awkward "take ownership of a `Results` and then return it unchanged" pattern. - `MaybeRequiresStorage` can borrow the `MaybeBorrowedLocals` results, avoiding two `clone` calls: one in `check_for_move` and one in `locals_live_across_suspend_points`. - `Results` no longer needs to derive `Clone`. This reinstates the cursor flexibility that was lost in rust-lang#118230 -- which removed the old `ResultsRefCursor` and `ResultsCloneCursor` types -- but in a *much* simpler way. Hooray!

rust-timer · 2024-10-24T07:17:43Z

Finished benchmarking commit (1c188eb): comparison URL.

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

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-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	0.8%	[0.4%, 1.0%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	2
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	1

Max RSS (memory usage)

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

Cycles

Results (primary -2.2%)

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

Binary size

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

Bootstrap: 781.235s -> 781.882s (0.08%)
Artifact size: 333.60 MiB -> 333.62 MiB (0.01%)

Because the default method is also a no-op.

nnethercote · 2024-10-24T08:27:55Z

The await-call-tree regression looks like it might be real. I ran Cachegrind locally but it wasn't informative. A bunch of functions got more instructions, a bunch got fewer, and there was no clear pattern. The regression is so small and only on a stress test so I don't think it's important.

@rustbot label: +perf-regression-triaged

compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs

Because creating a new cursor in every `check_for_move` call might results in quadratic behaviour. The cursor must be wrapped in a `RefCell`, now that analyses are passed everywhere by `&` instead of `&mut`.

First, remove the `self` param, which is unnecessary. Also, in `MaybeRequiresStorage::apply_before_statement_effect`, call `transfer_function` directly, as is already done in `MaybeRequiresStorage::apply_before_terminator_effect`. This makes it clear that the operation doesn't rely on the `MaybeBorrowedLocals` results.

nnethercote · 2024-10-28T02:03:31Z

I have added two new commits: one that reinstates the cursor within MaybeRequireStorage, and one that does minor cleanups.

nnethercote · 2024-10-29T22:48:32Z

@Jarcho talked on Zulip about needing analysis mutability for some Clippy analyses. So I will close this PR and redo the non-mutability bits in other PRs.

nnethercote added 2 commits October 23, 2024 17:15

Reduce some visibilities.

c4c2d2c

In MaybeRequiresStorage, store Results instead of ResultsCursor.

52b90f7

This requires a `clone` call in `check_for_move`, but makes `MaybeRequiresStorage` an immutable analysis, which enables the next commit.

rustbot assigned cjgillot Oct 23, 2024

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 Oct 23, 2024

nnethercote changed the title ~~Still more rustc mir dataflow cleanups~~ Still more rustc_mir_dataflow cleanups Oct 23, 2024

nnethercote marked this pull request as ready for review October 23, 2024 10:22

Make analyses and visitables immutable.

afbffe8

Much nicer.

nnethercote force-pushed the still-more-rustc_mir_dataflow-cleanups branch from 67d7730 to 9e735ca Compare October 23, 2024 23:38

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 Oct 23, 2024

This comment has been minimized.

Sign in to view

nnethercote added 7 commits October 24, 2024 15:54

Rename BlockFormatter::results as BlockFormatter::cursor.

dd24e0e

Because it's a `ResultsCursor`, not a `Results`.

Return label from write_node_label.

6a29ce8

Instead of mutating an empy label. Because it's conceptually simpler.

Remove Engine.

41b75a3

It's no longer needed. `Engine::iterate_to_fixpoint` can be inlined into `Analysis::iterate_to_fixpoint` and removed. The commit also renames `engine.rs` as `results.rs`.

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 24, 2024

Remove an unnecessary method.

d33eb49

Because the default method is also a no-op.

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 24, 2024

nnethercote force-pushed the still-more-rustc_mir_dataflow-cleanups branch from 9e735ca to d33eb49 Compare October 24, 2024 08:28

Remove some more unnecessary mut usage.

2b44ac2

cjgillot reviewed Oct 26, 2024

View reviewed changes

compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs Outdated Show resolved Hide resolved

cjgillot 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 Oct 27, 2024

nnethercote added 2 commits October 28, 2024 10:04

Reinstate ResultsCursor in MaybeRequiresStorage.

514f30f

Because creating a new cursor in every `check_for_move` call might results in quadratic behaviour. The cursor must be wrapped in a `RefCell`, now that analyses are passed everywhere by `&` instead of `&mut`.

nnethercote closed this Oct 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Still more `rustc_mir_dataflow` cleanups #132062

Still more `rustc_mir_dataflow` cleanups #132062

nnethercote commented Oct 23, 2024

rustbot commented Oct 23, 2024

nnethercote commented Oct 23, 2024

This comment has been minimized.

bors commented Oct 23, 2024

bors commented Oct 24, 2024

This comment has been minimized.

rust-timer commented Oct 24, 2024

nnethercote commented Oct 24, 2024

nnethercote commented Oct 28, 2024

nnethercote commented Oct 29, 2024

Still more rustc_mir_dataflow cleanups #132062

Still more rustc_mir_dataflow cleanups #132062

Conversation

nnethercote commented Oct 23, 2024

rustbot commented Oct 23, 2024

nnethercote commented Oct 23, 2024

This comment has been minimized.

bors commented Oct 23, 2024

bors commented Oct 24, 2024

This comment has been minimized.

rust-timer commented Oct 24, 2024

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

Instruction count

Max RSS (memory usage)

Cycles

Binary size

nnethercote commented Oct 24, 2024

nnethercote commented Oct 28, 2024

nnethercote commented Oct 29, 2024

Still more `rustc_mir_dataflow` cleanups #132062

Still more `rustc_mir_dataflow` cleanups #132062