I can't stop writing copy propagation passes #76723

jonas-schievink · 2020-09-15T00:39:36Z

This implements a simple and easy to understand intra-block copy propagation pass. It can clean up temporaries introduced by MIR building and reduces the amount of MIR we end up with, mostly improving the CTFE and incremental compilation benchmarks.

The pass triggers ~120000 times on the whole libstd.

I have not yet looked for improvements in the generated code, but given that this is the first attempt at a copy propagation pass that does not come with compile time regressions this should already be worth it on its own. I don't expect any improvements in generated code since LLVM is already capable of optimizing out redundant copies within basic blocks.

jonas-schievink · 2020-09-15T00:39:52Z

@bors try @rust-timer queue

rust-timer · 2020-09-15T00:39:53Z

Awaiting bors try build completion

bors · 2020-09-15T00:40:07Z

⌛ Trying commit ce5fed7aeec86b6b49236cafdc2017f2edaebc77 with merge dd7c661df15bcb47908d8fe078481eea3ab62938...

bors · 2020-09-15T01:26:01Z

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

rust-timer · 2020-09-15T01:26:03Z

Queued dd7c661df15bcb47908d8fe078481eea3ab62938 with parent 9b41541, future comparison URL.

rust-timer · 2020-09-15T03:25:30Z

Finished benchmarking try commit (dd7c661df15bcb47908d8fe078481eea3ab62938): 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

jyn514 · 2020-09-15T04:32:26Z

Oh no :(

mati865 · 2020-09-15T07:34:23Z

1684.1% increased time in MIR optimisations.

leonardo-m · 2020-09-15T08:31:55Z

Are those compile times (or run times of benchmarks)?

mati865 · 2020-09-15T08:34:54Z

Compile times of the benchmarks.

jonas-schievink · 2020-09-15T10:11:23Z

Oh, the clearing has O(l * b) performance for l locals and b basic blocks, will fix

jonas-schievink · 2020-09-15T21:07:48Z

@bors try @rust-timer queue

rust-timer · 2020-09-15T21:07:49Z

Awaiting bors try build completion

bors · 2020-09-15T21:07:59Z

⌛ Trying commit d976615e8bfe62e6a466046aaba45c17d672791f with merge 56e90b1cdbec60d65cf2a4d28e60ede43c1a1880...

bors · 2020-09-15T21:51:03Z

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

rust-timer · 2020-09-15T21:51:05Z

Queued 56e90b1cdbec60d65cf2a4d28e60ede43c1a1880 with parent 07ece44, future comparison URL.

rust-timer · 2020-09-15T23:26:30Z

Finished benchmarking try commit (56e90b1cdbec60d65cf2a4d28e60ede43c1a1880): 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

jonas-schievink · 2020-09-15T23:29:08Z

Much better. The remaining regressions seem to be caused by codegen unit partitioning from what I can tell.

leonardo-m · 2020-09-16T06:36:54Z

Compile times of the benchmarks.

Do you have some run-time benchmarks too to see the effect of this pull?

matprec · 2020-09-16T08:12:30Z

@leonardo-m
I wouldn't expect severe run-time effects of this pull request, because llvm should have optimized this before.
It's reducing the compile time because we're reducing the amount of LLVM-IR it has to check/optimize, and this is doing it on the MIR, so pre-monomorphization, which is more effective then on the monomorphized LLVM-IR.

jonas-schievink · 2020-09-18T22:58:36Z

It's reducing the compile time because we're reducing the amount of LLVM-IR it has to check/optimize, and this is doing it on the MIR, so pre-monomorphization, which is more effective then on the monomorphized LLVM-IR.

The improvements are almost entirely in incremental builds and the CTFE stress test, so it looks like we're only benefiting from the reduction of MIR, not LLVM IR (less metadata to write/read = faster incremental builds; less MIR to interpret = faster CTFE). Which is still good, of course, but it won't benefit from-scratch builds.

bors · 2020-09-26T04:15:44Z

☔ The latest upstream changes (presumably #76176) 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 · 2020-10-16T15:03:13Z

r? @oli-obk

bors · 2020-10-17T19:50:45Z

☔ The latest upstream changes (presumably #77373) 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

Auto-import made a bit of a mess here

Co-authored-by: LingMan <LingMan@users.noreply.github.com>

jonas-schievink · 2020-10-18T00:08:46Z

compiler/rustc_mir/src/transform/basic_block_copy_prop.rs

+        //   _2 = _1;
+        //   use(move _2);   <- can replace with `use(_1)`
+        // Or:
+        //   use(move _2, move _1); <- can replace with `use(_1, _1)`


Hmm, this doesn't sound right, this is only correct if the places _1 and _1 are used are allowed to overlap

Are there cases where they are not?

From codegen perspective it seemed correct to me when I was reviewing the changes. Based on the fact that codegen for Operand::Copy introduces a copy when passing by ref, unlike Operand::Move which does not.

Yeah, for example call destinations must not alias any call arguments (maybe this only applies to copied arguments, as you mentioned). I ran into this when writing the destination propagation pass.

Unfortunately these invariants haven't really been consciously designed or documented from what I know, but the MIR validator does try to enforce them (or what I think is a reasonably approximation of them).

It seems that this pass is still correct though – aliasing restrictions generally only apply when one of the places is being mutated, and when that happens we invalidate all affected locals.

Extended the comment

There is also implicit assumption about when the move invalidates the local. As far as I can see this pass could make a following transformation:

_2 = _1 _3 = f(move _1, move _2);

_2 = _1 _3 = f(move _1, _1);

EDIT: Either way it it looks OK, just for far more subtle reasons.

But generally speaking the aliasing concerns do apply to the inputs alone in the move case. For example, running this optimization and then just before codegen turning copy operands into move operands results in the same miscompilation that affected copy propagation.

Oof, yeah, that doesn't sound like it should work, but maybe it currently does. Promotion of copies to moves is something we do want to do at some point, so that would have to agree with this pass.

jonas-schievink · 2020-10-19T23:15:01Z

compiler/rustc_mir/src/transform/basic_block_copy_prop.rs

+            if let Some(local) = place.as_local() {
+                if let Some(known_place) = self.local_values.get(local) {
+                    debug!("{:?}: replacing use of {:?} with {:?}", location, place, known_place);
+                    *operand = Operand::Copy(known_place);


This "demotion" from move to copy could be a problem, since moves are treated quite differently and tend to optimize better. Not sure how to solve that without a pass that promotes them again (which needs dataflow).

we'll probably want such a pass anyway to invoke every now and then in the pipeline (and at the end of it).

If it's too expensive, we could do some caching similar to the predecessor cache.

oli-obk · 2020-10-20T08:20:20Z

...otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyBranches-final.after.diff

-+         (_4.0: &ViewportPercentageLength) = _1; // scope 0 at $DIR/early_otherwise_branch_68867.rs:22:15: 22:16
+          StorageLive(_4);                 // scope 0 at $DIR/early_otherwise_branch_68867.rs:22:14: 22:24
+          StorageLive(_5);                 // scope 0 at $DIR/early_otherwise_branch_68867.rs:22:15: 22:16
+          _5 = _1;                         // scope 0 at $DIR/early_otherwise_branch_68867.rs:22:15: 22:16
          StorageLive(_6);                 // scope 0 at $DIR/early_otherwise_branch_68867.rs:22:18: 22:23
          _6 = _2;                         // scope 0 at $DIR/early_otherwise_branch_68867.rs:22:18: 22:23


Hmm... I was wondering why these were left around, but then I realized SimplifyBranches happens before SimplifyLocals

oli-obk · 2020-10-20T08:26:31Z

src/test/mir-opt/simplify_try.try_identity.SimplifyLocals.after.mir

@@ -23,11 +24,14 @@ fn try_identity(_1: std::result::Result<u32, i32>) -> std::result::Result<u32, i
        }
    }
    scope 6 {
-        debug self => _0;                // in scope 6 at $SRC_DIR/core/src/result.rs:LL:COL
+        debug self => _2;                // in scope 6 at $SRC_DIR/core/src/result.rs:LL:COL


This seems like a regression in debugging. Once that local is optimized out, there will be no more debug info for self.

oli-obk · 2020-10-20T08:28:51Z

compiler/rustc_mir/src/transform/basic_block_copy_prop.rs

+/// Stores the locals that need to be invalidated when this local is modified or deallocated.
+#[derive(Default, Clone)]
+struct Invalidation {
+    locals: SmallVec<[Local; 4]>,


Please document this magic number (and maybe move to a constant?)

oli-obk · 2020-10-20T08:32:46Z

compiler/rustc_mir/src/transform/basic_block_copy_prop.rs

+        // *again* later on would be a use-after-move. For example:
+        //   _1 = ...;
+        //   _2 = move _1;
+        //   use(move _2);   <- can *not* replace with `use(move _1)` or `use(_1)`


Is this expected to get changed in the future if _2 is only ever used right here?

oli-obk · 2020-10-20T08:34:38Z

compiler/rustc_mir/src/transform/basic_block_copy_prop.rs

+
+        // (*) Some MIR statements and terminators may forbid aliasing between some of the places
+        // they access. This only happens between output and input places, never between multiple
+        // input places, so what this pass is safe: Any local that is mutated will invalidate all


Suggested change

// input places, so what this pass is safe: Any local that is mutated will invalidate all

// input places, so what this pass does is safe: Any local that is mutated will invalidate all

oli-obk · 2020-10-20T08:36:02Z

compiler/rustc_mir/src/transform/basic_block_copy_prop.rs

+        // Or:
+        //   use(move _2, move _1); <- can replace with `use(_1, _1)`, if the inputs may alias (*)
+        if let Operand::Copy(place) | Operand::Move(place) = operand {
+            if let Some(local) = place.as_local() {


we should be able to apply this to all places that satisfy place_eligible, right? Maybe not in this PR, but if nothing speaks against it, leave a FIXME, and if something does, please document it.

oli-obk · 2020-10-20T08:37:12Z

compiler/rustc_mir/src/transform/basic_block_copy_prop.rs

+            if let Some(local) = place.as_local() {
+                if let Some(known_place) = self.local_values.get(local) {
+                    debug!("{:?}: replacing use of {:?} with {:?}", location, place, known_place);
+                    *operand = Operand::Copy(known_place);


we'll probably want such a pass anyway to invoke every now and then in the pipeline (and at the end of it).

oli-obk · 2020-10-20T08:38:19Z

compiler/rustc_mir/src/transform/basic_block_copy_prop.rs

+            if let Some(local) = place.as_local() {
+                if let Some(known_place) = self.local_values.get(local) {
+                    debug!("{:?}: replacing use of {:?} with {:?}", location, place, known_place);
+                    *operand = Operand::Copy(known_place);


If it's too expensive, we could do some caching similar to the predecessor cache.

bors · 2020-10-26T21:28:52Z

☔ The latest upstream changes (presumably #68965) 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

camelid · 2020-12-04T02:24:58Z

@jonas-schievink Could you address Oli's review? Thanks for working on these opts!

crlf0710 · 2021-01-15T12:09:48Z

@jonas-schievink Ping from triage, closing this due to inactivity. Please feel free to reopen or create a new PR when you're ready to work on this again. Thanks!

jyn514 added A-mir-opt Area: MIR optimizations T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 15, 2020

jyn514 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-perf Status: Waiting on a perf run to be completed. labels Sep 15, 2020

jonas-schievink marked this pull request as ready for review September 17, 2020 19:33

This comment has been minimized.

Sign in to view

camelid 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 16, 2020

rust-highfive assigned oli-obk Oct 16, 2020

jonas-schievink added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 17, 2020

jonas-schievink and others added 9 commits October 17, 2020 22:16

Add an intra-basic-block copy propagation pass

8393037

Ensure linear time complexity

bfb363d

Clean up and document

91131e5

Don't MIR-optimize the codegen tests

63c0464

Clean up imports

187a7c0

Auto-import made a bit of a mess here

Reorder invalidation and replacement steps

f189785

Fix rebase fallout

ed37dbf

Update compiler/rustc_middle/src/mir/visit.rs

91b4c55

Co-authored-by: LingMan <LingMan@users.noreply.github.com>

Bless mir-opt tests

cd01d16

jonas-schievink commented Oct 18, 2020

View reviewed changes

Extend comment

cf50138

jonas-schievink commented Oct 19, 2020

View reviewed changes

oli-obk requested changes Oct 20, 2020

View reviewed changes

oli-obk 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 23, 2020

crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 13, 2020

crlf0710 closed this Jan 15, 2021

jonas-schievink mentioned this pull request Jul 14, 2021

Local copy propagation #87126

Closed

	// input places, so what this pass is safe: Any local that is mutated will invalidate all
	// input places, so what this pass does is safe: Any local that is mutated will invalidate all

I can't stop writing copy propagation passes #76723

I can't stop writing copy propagation passes #76723

Conversation

jonas-schievink commented Sep 15, 2020 • edited Loading

jonas-schievink commented Sep 15, 2020

rust-timer commented Sep 15, 2020

bors commented Sep 15, 2020

bors commented Sep 15, 2020

rust-timer commented Sep 15, 2020

rust-timer commented Sep 15, 2020

jyn514 commented Sep 15, 2020

mati865 commented Sep 15, 2020

leonardo-m commented Sep 15, 2020

mati865 commented Sep 15, 2020

jonas-schievink commented Sep 15, 2020

jonas-schievink commented Sep 15, 2020

rust-timer commented Sep 15, 2020

bors commented Sep 15, 2020

bors commented Sep 15, 2020

rust-timer commented Sep 15, 2020

rust-timer commented Sep 15, 2020

jonas-schievink commented Sep 15, 2020

leonardo-m commented Sep 16, 2020

matprec commented Sep 16, 2020 • edited Loading

This comment has been minimized.

jonas-schievink commented Sep 18, 2020

bors commented Sep 26, 2020

Dylan-DPC-zz commented Oct 16, 2020

bors commented Oct 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmiasko Oct 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 26, 2020

camelid commented Dec 4, 2020

crlf0710 commented Jan 15, 2021

jonas-schievink commented Sep 15, 2020 •

edited

Loading

matprec commented Sep 16, 2020 •

edited

Loading

tmiasko Oct 18, 2020 •

edited

Loading