-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Streamline MIR dataflow cursors #118230
Streamline MIR dataflow cursors #118230
Conversation
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…ors, r=<try> Streamline MIR dataflow cursors r? `@ghost`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (d449aa5): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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 Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 675.697s -> 677.516s (0.27%) |
No change on perf, as expected. That's good news, it means the very small increase in cloning has no meaningful perf effect. |
And insert some whitespace.
It's only implemented for analyses that implement `Copy`, which means it's basically a complicated synonym for `Copy`. So this commit removes it and uses `Copy` directly. (That direct use will be removed in a later commit.)
It's currently used because `requires_storage_results` is used in two locations: once with a cursor, and once later on without a cursor. The non-consuming `as_results_cursor` is used for the first location. But we can instead use the consuming `into_results_cursor` and then use `into_results` to extract the `Results` from the finished-with cursor for use at the second location.
The new code is a little clunky, but I couldn't see how to make it better.
It's no longer used.
By just cloning the entire `Results` in the one place where `ResultsClonedCursor` was used. This is extra allocations but the performance effect is negligible.
They're now unused.
They both now only ever contain a `Results<'tcx, A>`. This means `AnalysisResults` can be removed, as can many `borrow`/`borrow_mut` calls. Also `Results` no longer needs a `PhantomData` because `'tcx` is now named by `entry_sets`.
0e0ffbd
to
e966c89
Compare
cc @Jarcho |
Looked back at what this was before I changed it. The current mess of different cursor types was just a way to avoid cloning in all the cases it did before. It might be possible to simplify things by only borrowing the entry sets and cloning the analysis. |
That's exactly what |
Since this doesn't have a big effect, I agree with the assessment that it doesn't really matter. |
The PR removes
The PR removes |
LGTM. Why is this still marked as draft? |
Sorry, I forgot to remove the draft marker :) |
@cjgillot: is your "LGTM" equivalent to an r+ ? |
@bors r+ |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
Finished benchmarking commit (317d14a): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 673.516s -> 673.262s (-0.04%) |
`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!
…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`
`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!
rustc_mir_dataflow
has two kinds of results (Results
andResultsCloned
) and three kinds of results cursor (ResultsCursor
,ResultsClonedCursor
,ResultsRefCursor
). I found this quite confusing.This PR removes
ResultsCloned
,ResultsClonedCursor
, andResultsRefCursor
, leaving justResults
andResultsCursor
. This makes the relevant code shorter and easier to read, and there is no performance penalty.r? @cjgillot