Remove unused discriminant reads from MIR bodies #70595

wesleywiser · 2020-03-31T01:45:43Z

Allow the SimplifyLocals pass to remove reads of discriminants if the
read is never used.

oli-obk · 2020-03-31T12:51:46Z

src/librustc_mir/transform/simplify.rs

@@ -382,6 +382,11 @@ impl<'a, 'tcx> Visitor<'tcx> for DeclMarker<'a, 'tcx> {
                            }
                        }
                    }
+                } else if let StatementKind::Assign(box (p, Rvalue::Discriminant(d))) = &stmt.kind {


Are there any Rvalues (except those that reference a ConstKind::Unevaluated, which I'm working on) that have side effects? I think we could use a visitor here looking for ConstKind::Unevaluated and remove the assignment if there is no Unevaluated in that Rvalue

Hmm... I don't think we have side effects in the sense that there might be a user-defined function call or something hiding in an Rvalue (excluding ConstKind::Unevaluated). However there are somethings that might be termed side effects like:

de-referencing a pointer

Removing a deref means we could potentially be removing UB if the pointer isn't actually valid. That seems ok to me.

Boxing values

I believe this lowers to a call to the global allocator which means you could observe this not happening via a custom allocator.

{Checked,}Binary ops could over/under flow.

In general, Rvalues except for these seem side-effect free to me (excluding ConstKind::Unevaluated and things that contain Place values with projections):

BinaryOp

CheckedBinaryOp

Cast

NullaryOp

UnaryOp

CheckedBinaryOp doesn't have a side-effect as over/under flow will just return that it had over/under flowed. UnaryOp is used to implement both negation and bitwise not. The later is always side-effect free. Cast(CatKind::Pointer(_), _, _) is side-effect free I think, but Cast(CastKind::Misc, _, _) could remove a ptr-int cast or int-ptr cast. NullaryOp(NullOp::SizeOf, ty) only needs to ensure that layout_of doesn't error on the type.

Boxing values

* I believe this lowers to a call to the global allocator which means you could observe this _not_ happening via a custom allocator.

Yes, it lowers to the exchange_malloc lang item, which uses Global.alloc:

rust/src/liballoc/alloc.rs

Line 225 in dd155df

match Global.alloc(layout) {

Removing this allocation would be equivalent to any function in liballoc or libstd changing when and how much it allocates though, so I don't think anybody can depend on it anyway.

src/test/mir-opt/simplify-locals-removes-unused-discriminant-reads.rs

eddyb · 2020-04-01T13:35:40Z

src/librustc_mir/transform/simplify.rs

@@ -382,6 +382,11 @@ impl<'a, 'tcx> Visitor<'tcx> for DeclMarker<'a, 'tcx> {
                            }
                        }
                    }
+                } else if let StatementKind::Assign(box (p, Rvalue::Discriminant(d))) = &stmt.kind {
+                    if !p.is_indirect() {
+                        trace!("skipping store of discriminant value {:?} to {:?}", d, p);


This confused me at first, but basically this is treating certain initializers as not keeping the destination alive, right?
Something very important in these checks though: you need to make sure p.local is local otherwise it may be somewhere else in the statement (you shouldn't trust ctx to let you know where it is, IMO).

Also, it would be nice if if let StatementKind::Assign(box (p, was only once, and then the only check for the Rvalue was whether we consider it to have side-effects.

Also, does this pass re-run itself to fixpoint? Because if you remove an assignment to a dead local, you remove uses of other locals also mentioned in that assignment, which may be their last.

You can't do this in one pass because e.g. in x = y, y might be unused only if x is unused, and you can't know that ahead of time.

basically this is treating certain initializers as not keeping the destination alive, right?

Yup, that's correct.

Something very important in these checks though: you need to make sure p.local is local otherwise it may be somewhere else in the statement (you shouldn't trust ctx to let you know where it is, IMO).

I'm struggling to think of a case where that would happen. Do you have an example in mind?

Also, does this pass re-run itself to fixpoint? Because if you remove an assignment to a dead local, you remove uses of other locals also mentioned in that assignment, which may be their last.

It doesn't currently. That would be a good improvement though.

I'm struggling to think of a case where that would happen. Do you have an example in mind?

The point is to check instead of assuming, especially in this case where it's basically free.
If nothing else it serves as self-documenting code.

oli-obk · 2020-04-01T14:16:01Z

@bors r+

Please open issues for

fixed-point running of the optimization (or finding an algorithm that works without just looping the entire thing)
generalizing to all rvalues

bors · 2020-04-01T14:16:03Z

📌 Commit bd99533f18134d6ab4b2dbebcf2844a351a22c45 has been approved by oli-obk

bors · 2020-04-01T14:16:04Z

🌲 The tree is currently closed for pull requests below priority 500, this pull request will be tested once the tree is reopened

eddyb · 2020-04-01T16:03:04Z

@bors r- I still want the change for checking p.local == local.

Allow the `SimplifyLocals` pass to remove reads of discriminants if the read is never used.

oli-obk · 2020-04-02T12:25:11Z

@bors r+

bors · 2020-04-02T12:25:12Z

📌 Commit 75e2e8c has been approved by oli-obk

@oli-obk

…ant_reads, r=oli-obk Remove unused discriminant reads from MIR bodies Allow the `SimplifyLocals` pass to remove reads of discriminants if the read is never used. Fixes rust-lang#70531 r? @oli-obk

@ghost

Rollup of 7 pull requests Successful merges: - rust-lang#70487 (Stabilize float::to_int_unchecked) - rust-lang#70595 (Remove unused discriminant reads from MIR bodies) - rust-lang#70691 (Improve docs in `AllocRef`) - rust-lang#70694 (Use Self over specific type in return position) - rust-lang#70700 (Expand on platform details of `include_xxx` macros) - rust-lang#70708 (Fix typo in u8::to_ascii_uppercase and u8::to_ascii_lowercase) - rust-lang#70716 (Unerase regions in infer_placeholder_type) Failed merges: r? @ghost

…c_boogaloo, r=oli-obk [mir-opt] Run SimplifyLocals to a fixedpoint and handle most rvalues Follow up to review feedback left on rust-lang#70595.

rust-highfive assigned oli-obk Mar 31, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2020

oli-obk reviewed Mar 31, 2020

View reviewed changes

src/test/mir-opt/simplify-locals-removes-unused-discriminant-reads.rs Outdated Show resolved Hide resolved

wesleywiser force-pushed the remove_unused_discriminant_reads branch from 90732cf to bd99533 Compare April 1, 2020 12:48

eddyb reviewed Apr 1, 2020

View reviewed changes

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 Apr 1, 2020

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 1, 2020

Remove unused discriminant reads from MIR bodies

75e2e8c

Allow the `SimplifyLocals` pass to remove reads of discriminants if the read is never used.

wesleywiser force-pushed the remove_unused_discriminant_reads branch from bd99533 to 75e2e8c Compare April 2, 2020 12:14

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

Centril mentioned this pull request Apr 2, 2020

Rollup of 8 pull requests #70711

Closed

Centril mentioned this pull request Apr 2, 2020

Rollup of 7 pull requests #70722

Merged

bors merged commit 4cba69e into rust-lang:master Apr 3, 2020

wesleywiser mentioned this pull request Apr 15, 2020

[mir-opt] Run SimplifyLocals to a fixedpoint and handle most rvalues #70755

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unused discriminant reads from MIR bodies #70595

Remove unused discriminant reads from MIR bodies #70595

wesleywiser commented Mar 31, 2020

oli-obk Mar 31, 2020

wesleywiser Mar 31, 2020

bjorn3 Apr 1, 2020

eddyb Apr 1, 2020

wesleywiser Apr 1, 2020

eddyb Apr 1, 2020

oli-obk commented Apr 1, 2020

bors commented Apr 1, 2020

bors commented Apr 1, 2020

eddyb commented Apr 1, 2020

oli-obk commented Apr 2, 2020

bors commented Apr 2, 2020

Remove unused discriminant reads from MIR bodies #70595

Remove unused discriminant reads from MIR bodies #70595

Conversation

wesleywiser commented Mar 31, 2020

oli-obk Mar 31, 2020

Choose a reason for hiding this comment

wesleywiser Mar 31, 2020

Choose a reason for hiding this comment

bjorn3 Apr 1, 2020

Choose a reason for hiding this comment

eddyb Apr 1, 2020

Choose a reason for hiding this comment

wesleywiser Apr 1, 2020

Choose a reason for hiding this comment

eddyb Apr 1, 2020

Choose a reason for hiding this comment

oli-obk commented Apr 1, 2020

bors commented Apr 1, 2020

bors commented Apr 1, 2020

eddyb commented Apr 1, 2020

oli-obk commented Apr 2, 2020

bors commented Apr 2, 2020