rustc_mir: handle all aggregate kinds in, and always run, the deaggregator. #48052

eddyb · 2018-02-07T15:30:23Z

This helps with removingRvalue::Aggregate from the MIR, and with enabling more optimizations.
r? @nikomatsakis

eddyb · 2018-02-07T15:32:16Z

This might be a bit slow to run, let's see:

@bors try

bors · 2018-02-07T15:32:29Z

⌛ Trying commit fda2441f1b34fd6135e7af93aec859ad7e4434ad with merge 23ae5261b60c2cf53547176a3b625359c00f6a5f...

eddyb · 2018-02-07T16:41:26Z

UI tests still broken on master, concealed test that needed to be updated.

@bors try

bors · 2018-02-07T16:41:37Z

⌛ Trying commit 5cc4b94 with merge 7d831d0...

@nikomatsakis

rustc_mir: handle all aggregate kinds in, and always run, the deaggregator. This helps with removing`Rvalue::Aggregate` from the MIR, and with enabling more optimizations. r? @nikomatsakis

nikomatsakis · 2018-02-07T17:30:09Z

src/librustc_mir/transform/deaggregator.rs

-        if tcx.sess.opts.debugging_opts.mir_opt_level <= 2 {
-            return;
-        }
-
        // Don't run on constant MIR, because trans might not be able to
        // evaluate the modified MIR.
        // FIXME(eddyb) Remove check after miri is merged.
        let id = tcx.hir.as_local_node_id(source.def_id).unwrap();
        match (tcx.hir.body_owner_kind(id), source.promoted) {


Nit: I find this match a bit hard to read. Maybe something like this?

let is_constant = source.promoted.is_some() || match tcx.hir.body_owner_kind(id) { hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(_) => true, hir::BodyOwnerKind::Fn => tcx.is_const_fn(source.def_id), }; if is_constant { return; }

It's copied from elsewhere, this pattern is used in several passes - and this will hopefully go away ~soon anyway.

nikomatsakis · 2018-02-07T17:34:20Z

src/librustc_mir/transform/deaggregator.rs

+            while let Some(i) = bb.statements[start..].iter().position(&can_deaggregate) {
+                let i = start + i;
+
+                // FIXME(eddyb) this is probably more expensive than it should be.


Seems fine, you could also drain() and accumulate into a fresh vector or something...

let mut new_statements = vec![]; for statement in bb.statements.drain(..) { if !can_deaggregate(statement) { new_statements.push(statement); } ... }

though probably you would want to test first that there is at least one match.

nikomatsakis · 2018-02-07T17:37:50Z

src/librustc_mir/transform/deaggregator.rs

+                        lhs.clone().elem(ProjectionElem::ConstantIndex {
+                            offset,
+                            // FIXME(eddyb) `min_length` doesn't appear to be used.
+                            min_length: offset + 1,


It was intended to be used for slice patterns by borrow check, I believe (and perhaps to handle from_end).

This is still a WIP =)

nikomatsakis · 2018-02-07T17:39:33Z

r=me modulo that one nit about the match

eddyb · 2018-02-07T17:50:16Z

@nikomatsakis I don't want to merge with the last commit (which turns this on by default) without perf results - if this is too slow we'd have to optimize it first or wait until we can remove Aggregate.

Zoxc · 2018-02-07T18:50:38Z

src/librustc_mir/transform/mod.rs

+        // Lowering generator control-flow and variables
+        // has to happen before we do anything else to them.
+        generator::StateTransform,
+


We want to lower generator as late as possible, in particular we want to run at least SimplifyLocals before the StateTransform. Did you move this up because you triggered the sanity checks in StateTransform?

Yeah, those sanity checks don't survive past optimizations.

bors · 2018-02-07T18:51:50Z

☀️ Test successful - status-travis
State: approved= try=True

eddyb · 2018-02-07T18:52:21Z

@Mark-Simulacrum Can I get a perf run of this?

eddyb · 2018-02-07T19:25:01Z

I can reproduce the Travis failure (which didn't trigger on the @bors run) locally - and if I change

rust/src/test/run-make/sepcomp-cci-copies/Makefile

Line 8 in fee39ba

$(RUSTC) foo.rs --emit=llvm-ir -C codegen-units=3 \

from 3 to 4 it passes. With only 3 CGUs, some may get merged (based on MIR complexity?)

rust/src/librustc_mir/monomorphize/partitioning.rs

Line 52 in fee39ba

//! - There are two codegen units for every source-level module:

According to this comment, the test is wrong and it should use 6 to ensure one CGU per module.
I'll push a fix for the test.

eddyb · 2018-02-08T00:01:10Z

The perf results don't look great for deep-vector - if you open it, you'll see why: it has a very large vec![...], which internally uses box [...] and now every single of those fields gets deaggregated.

#48052 (comment) seems irrelevant, it's really just slowed down by dealing with individual element destinations. @nikomatsakis Do we defer on array deaggregation, or take the hit?

nikomatsakis · 2018-02-08T16:59:25Z

@eddyb maybe we should set a threshold? (e.g., deaggregate up to length 8 or 16 or something?)

eddyb · 2018-02-08T20:21:10Z

@nikomatsakis That's not as useful because my eventual plan is to just kill off Rvalue::Aggregate.

nikomatsakis · 2018-02-13T18:59:41Z

So @eddyb and I discussed the deep-vector slowdown over IRC. They are hoping that they can remove it by altering the structure of MIR in various ways to make it more efficient -- e.g., to simplify places, and hence flatten statements so that they don't require allocation.

My current feeling is that we should insert a threshold check for now so that deep-vector doesn't regress, and then re-evaluate as we make those changes. If we have to swallow a compilation time hit on this use case, so be it, but I'd rather wait until later.

Also, I'd like to see some perf benchmarks covering big constants that are not vec![] (e.g., &[...], include!, etc). Not sure if we have those.

cc @rust-lang/compiler -- thoughts?

eddyb · 2018-02-13T19:12:43Z

My position is that we should just not do arrays for now if that regresses too much.

nikomatsakis · 2018-02-13T20:59:38Z

@eddyb that seems fine too, let's just open a FIXME

nikomatsakis · 2018-02-13T21:00:39Z

r=me with that done

eddyb · 2018-02-14T14:10:55Z

@bors try

bors · 2018-02-14T14:11:05Z

⌛ Trying commit b91a7e5 with merge 4b24c56...

@nikomatsakis

rustc_mir: handle all aggregate kinds in, and always run, the deaggregator. This helps with removing`Rvalue::Aggregate` from the MIR, and with enabling more optimizations. r? @nikomatsakis

eddyb · 2018-02-21T12:35:46Z

@bors retry

update failed

bors · 2018-02-22T00:04:19Z

⌛ Testing commit c9fcede with merge a63e5c3a0377366521072aa3c6bfc5f9484efb52...

bors · 2018-02-22T03:01:48Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a63e5c3a0377366521072aa3c6bfc5f9484efb52 to master...

eddyb · 2018-02-22T12:07:29Z

Do I need to rebase or something?...

kennytm · 2018-02-22T12:39:52Z

@bors retry

It seems we had accidentally revoked bors's push permission, thus the failure to merge.

bors · 2018-02-22T14:23:06Z

⌛ Testing commit c9fcede with merge 625d01a26b2254ee9cd54bf206b7b3c6e74ffbbb...

bors · 2018-02-22T17:24:00Z

💔 Test failed - status-appveyor

kennytm · 2018-02-22T17:28:50Z

@bors retry #46903

bors · 2018-02-22T20:25:01Z

⌛ Testing commit c9fcede with merge a9099f59b5a5d7f9ffb8b5b96f8c4757601c9e9c...

bors · 2018-02-22T23:25:36Z

💔 Test failed - status-appveyor

kennytm · 2018-02-22T23:59:20Z

@bors retry #46903

bors · 2018-02-23T02:21:12Z

⌛ Testing commit c9fcede with merge 9284353...

@nikomatsakis

rustc_mir: handle all aggregate kinds in, and always run, the deaggregator. This helps with removing`Rvalue::Aggregate` from the MIR, and with enabling more optimizations. r? @nikomatsakis

bors · 2018-02-23T05:19:43Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 9284353 to master...

rust-highfive assigned nikomatsakis Feb 7, 2018

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 7, 2018

eddyb force-pushed the deggregate branch from fda2441 to 5cc4b94 Compare February 7, 2018 16:36

nikomatsakis approved these changes Feb 7, 2018

View reviewed changes

Zoxc reviewed Feb 7, 2018

View reviewed changes

pietroalbini 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 Feb 12, 2018

eddyb force-pushed the deggregate branch from 5a76af8 to 8a09787 Compare February 13, 2018 13:20

eddyb mentioned this pull request Feb 13, 2018

Deaggregate arrays not just structs/enums/tuples/closures. #48193

Closed

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 21, 2018

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 Feb 21, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 22, 2018

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 Feb 22, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 22, 2018

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 Feb 22, 2018

bors merged commit c9fcede into rust-lang:master Feb 23, 2018

eddyb deleted the deggregate branch February 23, 2018 15:18

rustc_mir: handle all aggregate kinds in, and always run, the deaggregator. #48052

rustc_mir: handle all aggregate kinds in, and always run, the deaggregator. #48052

Conversation

eddyb commented Feb 7, 2018

eddyb commented Feb 7, 2018

bors commented Feb 7, 2018

eddyb commented Feb 7, 2018

bors commented Feb 7, 2018

nikomatsakis Feb 7, 2018

Choose a reason for hiding this comment

eddyb Feb 7, 2018

Choose a reason for hiding this comment

nikomatsakis Feb 7, 2018

Choose a reason for hiding this comment

nikomatsakis Feb 7, 2018

Choose a reason for hiding this comment

nikomatsakis Feb 7, 2018

Choose a reason for hiding this comment

nikomatsakis commented Feb 7, 2018

eddyb commented Feb 7, 2018

Zoxc Feb 7, 2018

Choose a reason for hiding this comment

eddyb Feb 7, 2018

Choose a reason for hiding this comment

bors commented Feb 7, 2018

eddyb commented Feb 7, 2018

eddyb commented Feb 7, 2018

eddyb commented Feb 8, 2018 • edited Loading

nikomatsakis commented Feb 8, 2018 • edited Loading

eddyb commented Feb 8, 2018

nikomatsakis commented Feb 13, 2018

eddyb commented Feb 13, 2018

nikomatsakis commented Feb 13, 2018

nikomatsakis commented Feb 13, 2018

eddyb commented Feb 14, 2018

bors commented Feb 14, 2018

eddyb commented Feb 21, 2018

bors commented Feb 22, 2018

bors commented Feb 22, 2018

eddyb commented Feb 22, 2018

kennytm commented Feb 22, 2018

bors commented Feb 22, 2018

bors commented Feb 22, 2018

kennytm commented Feb 22, 2018

bors commented Feb 22, 2018

bors commented Feb 22, 2018

kennytm commented Feb 22, 2018

bors commented Feb 23, 2018

bors commented Feb 23, 2018

eddyb commented Feb 8, 2018 •

edited

Loading

nikomatsakis commented Feb 8, 2018 •

edited

Loading