MIR opt: Expand aggregates into multiple locals #85796

cjgillot · 2021-05-28T22:50:41Z

The MIR deaggregator expands aggregate assignments into field accesses, downcasts and discriminant assignments. However, this creates needlessly complicated place projections that may limit later optimization passes.

This PR attempts to replace those accesses by new locals that can be optimized more easily, for instance by DestProp.

rust-highfive · 2021-05-28T22:50:44Z

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

erikdesjardins · 2021-05-29T18:10:58Z

Related: #48300 was a previous attempt at this

compiler/rustc_mir/src/transform/flatten_locals.rs

bjorn3 · 2021-05-29T19:47:18Z

compiler/rustc_mir/src/transform/flatten_locals.rs

+        }
+
+        fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, location: Location) {
+            if place.projection.is_empty()


I think we should play safe and reject all locals that have their address taken. Whether only for a part of as a whole. At least until we have better support for handling references in mir opts.

bors · 2021-05-30T17:46:27Z

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

varkor · 2021-06-06T12:05:36Z

I don't have time to review this at the moment. Going to reassign to r? @bjorn3 who's more familiar with this code.

bjorn3 · 2021-06-06T12:51:30Z

r? @RalfJung This optimization depends on the exact semantics of MIR and we have had miscompilations due to incorrect MIR optimizations before. As such I don't feel comfortable signing off on a MIR optimization.

RalfJung · 2021-06-06T13:23:44Z

I can help with advice on MIR semantics, but I won't have time to review a PR of this size any time soon I am afraid.

bjorn3 · 2021-06-06T13:44:19Z

No problem. I will try to taken review on myself then, but I would like it to either land behind either -Zmir-opt-level=3/4 or -Zunsound-mir-opts.

r? @bjorn3

bors · 2021-06-07T13:34:50Z

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

crlf0710 · 2021-06-26T15:37:58Z

Triage: Seems CI is still red.

rust-timer · 2021-07-25T09:14:42Z

Queued 2a09f2c20f1895c61a8f6f6a3a484627b2327ce4 with parent d9aa287, future comparison URL.

rust-timer · 2021-07-25T10:54:40Z

Finished benchmarking try commit (2a09f2c20f1895c61a8f6f6a3a484627b2327ce4): comparison url.

Summary: This change led to significant mixed results 🤷 in compiler performance.

Moderate improvement in instruction counts (up to -4.2% on incr-unchanged builds of deeply-nested-async-check)
Moderate regression in instruction counts (up to 1.4% on incr-patched: b9b3e592dd cherry picked builds of style-servo-opt)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this 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-review -S-waiting-on-perf +perf-regression

tmiasko · 2021-07-25T21:01:04Z

compiler/rustc_mir/src/transform/flatten_locals.rs

+                return;
+            }
+
+            match self.map.fields.entry(place.clone()) {


PlaceRef implements copy, so clone should be unnecessary.

tmiasko · 2021-07-25T21:04:06Z

compiler/rustc_mir/src/transform/mod.rs

+        &flatten_locals::FlattenLocals,
        &remove_storage_markers::RemoveStorageMarkers,


RemoveStorageMarkers removes the storage markers at opt-level=0. Reordering those two passes would avoid needles expansion of storage markers during flattening.

tmiasko · 2021-07-25T21:14:31Z

compiler/rustc_mir/src/transform/flatten_locals.rs

+                    return None;
+                }
+                let mut expanded = Vec::new();
+                for (k, &v) in &replacements.fields {


This seems to be quadratic in the MIR size in the worst case. Could we avoid that?

tmiasko · 2021-07-25T21:45:46Z

compiler/rustc_mir/src/transform/flatten_locals.rs

+                Some(expanded.into_iter())
+            });
+        }
+        super::simplify::simplify_locals(body, tcx);


Does running simplify locals immediately after flattening have additional benefits compared to leaving this clean-up for later?

tmiasko · 2021-07-25T23:14:11Z

Benchmarks that slightly regressed include check and doc builds. I would suspect that this is mostly a side effect of different partitioning while building rustc. It could be interesting to run perf comparison while keeping partitioning fixed, e.g., by always returning 1 from MonoItem::size_estimate (though, this would require quite a bit of extra work to prepare to builds for comparison).

bors · 2021-09-09T00:01:51Z

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

tmiasko · 2021-09-09T12:50:03Z

compiler/rustc_mir/src/transform/flatten_locals.rs

+                    StatementKind::StorageDead(l) => (false, *l),
+                    _ => return None,
+                };
+                replaced_locals.get(origin_local).map(move |final_locals| {


This removes storage markers for locals without any replacements (replaced_locals is an IndexVec now, so get doesn't have desired effect).

tmiasko · 2021-09-09T12:58:13Z

Other than the issue with storage markers, the implementation looks good to me. The perf results are mostly a regression, though?

Perform simple scalar replacement of aggregates (SROA) MIR opt This is a re-open of rust-lang#85796 I copied the debuginfo implementation (first commit) from `@eddyb's` own SROA PR. This pass replaces plain field accesses by simple locals when possible. To be eligible, the replaced locals: - must not be enums or unions; - must not be used whole; - must not have their address taken. The storage and deinit statements are duplicated on each created local. cc `@tmiasko` who reviewed the former version of this PR.

Perform simple scalar replacement of aggregates (SROA) MIR opt This is a re-open of rust-lang/rust#85796 I copied the debuginfo implementation (first commit) from `@eddyb's` own SROA PR. This pass replaces plain field accesses by simple locals when possible. To be eligible, the replaced locals: - must not be enums or unions; - must not be used whole; - must not have their address taken. The storage and deinit statements are duplicated on each created local. cc `@tmiasko` who reviewed the former version of this PR.

rust-highfive assigned varkor May 28, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2021

This comment has been minimized.

Sign in to view

cjgillot force-pushed the deagg branch from e382375 to 5ec080e Compare May 29, 2021 13:36

This comment has been minimized.

Sign in to view

bjorn3 reviewed May 29, 2021

View reviewed changes

compiler/rustc_mir/src/transform/flatten_locals.rs Outdated Show resolved Hide resolved

bjorn3 reviewed May 29, 2021

View reviewed changes

cjgillot force-pushed the deagg branch from 5ec080e to 4c2fc71 Compare May 29, 2021 19:47

This comment has been minimized.

Sign in to view

cjgillot force-pushed the deagg branch from 4c2fc71 to d551058 Compare June 2, 2021 14:31

This comment has been minimized.

Sign in to view

rust-highfive assigned bjorn3 and unassigned varkor Jun 6, 2021

rust-highfive assigned RalfJung and unassigned bjorn3 Jun 6, 2021

rust-highfive assigned bjorn3 and unassigned RalfJung Jun 6, 2021

cjgillot force-pushed the deagg branch from d551058 to ff8ef51 Compare June 8, 2021 19:31

This comment has been minimized.

Sign in to view

crlf0710 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2021

rustbot added the perf-regression Performance regression. label Jul 25, 2021

tmiasko reviewed Jul 25, 2021

View reviewed changes

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021

inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021

cjgillot force-pushed the deagg branch from d0b02ef to d3bab91 Compare August 29, 2021 14:11

This comment has been minimized.

Sign in to view

cjgillot force-pushed the deagg branch from 76578b6 to 3eb1586 Compare September 2, 2021 20:31

Flatten aggregates into locals.

c1accaf

cjgillot force-pushed the deagg branch from 3eb1586 to c1accaf Compare September 3, 2021 22:23

cjgillot mentioned this pull request Sep 3, 2021

Scalar replacement of enums #88628

Closed

tmiasko reviewed Sep 9, 2021

View reviewed changes

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2021

cjgillot closed this Oct 27, 2021

cjgillot mentioned this pull request Oct 2, 2022

Perform simple scalar replacement of aggregates (SROA) MIR opt #102570

Merged

cjgillot deleted the deagg branch February 13, 2023 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIR opt: Expand aggregates into multiple locals #85796

MIR opt: Expand aggregates into multiple locals #85796

cjgillot commented May 28, 2021

rust-highfive commented May 28, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

erikdesjardins commented May 29, 2021

bjorn3 May 29, 2021

This comment has been minimized.

bors commented May 30, 2021

This comment has been minimized.

varkor commented Jun 6, 2021

bjorn3 commented Jun 6, 2021

RalfJung commented Jun 6, 2021

bjorn3 commented Jun 6, 2021 •

edited

Loading

bors commented Jun 7, 2021

This comment has been minimized.

crlf0710 commented Jun 26, 2021

rust-timer commented Jul 25, 2021

rust-timer commented Jul 25, 2021

tmiasko Jul 25, 2021

tmiasko Jul 25, 2021

tmiasko Jul 25, 2021

tmiasko Jul 25, 2021

tmiasko commented Jul 25, 2021

This comment has been minimized.

This comment has been minimized.

bors commented Sep 9, 2021

tmiasko Sep 9, 2021

tmiasko commented Sep 9, 2021

		&flatten_locals::FlattenLocals,
		&remove_storage_markers::RemoveStorageMarkers,

MIR opt: Expand aggregates into multiple locals #85796

MIR opt: Expand aggregates into multiple locals #85796

Conversation

cjgillot commented May 28, 2021

rust-highfive commented May 28, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

erikdesjardins commented May 29, 2021

bjorn3 May 29, 2021

Choose a reason for hiding this comment

This comment has been minimized.

bors commented May 30, 2021

This comment has been minimized.

varkor commented Jun 6, 2021

bjorn3 commented Jun 6, 2021

RalfJung commented Jun 6, 2021

bjorn3 commented Jun 6, 2021 • edited Loading

bors commented Jun 7, 2021

This comment has been minimized.

crlf0710 commented Jun 26, 2021

rust-timer commented Jul 25, 2021

rust-timer commented Jul 25, 2021

tmiasko Jul 25, 2021

Choose a reason for hiding this comment

tmiasko Jul 25, 2021

Choose a reason for hiding this comment

tmiasko Jul 25, 2021

Choose a reason for hiding this comment

tmiasko Jul 25, 2021

Choose a reason for hiding this comment

tmiasko commented Jul 25, 2021

This comment has been minimized.

This comment has been minimized.

bors commented Sep 9, 2021

tmiasko Sep 9, 2021

Choose a reason for hiding this comment

tmiasko commented Sep 9, 2021

bjorn3 commented Jun 6, 2021 •

edited

Loading