Add optimization to avoid load of address

[simonvandel]

Look for the sequence

_2 = &_1;
...
_5 = (*_2)

in which we can replace the last statement with _5 = _1 to avoid the load of _2

[rust-highfive]

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

[simonvandel]

Hi @jonas-schievink, thanks for the review.
In the newest commits I:

• Rebased on master to fix conflict
• Added a lookback of 6 (the commit has a table for the numbers I got)
• Added a test and fixed the miscompilation you pointed out

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[oli-obk]

This optimization is very scary, it has lots of edge cases that we need to think about

[oli-obk]

if local_being_derefed is used on the rhs of an assignment to another local, your optimization may misfire I think.

    let x = 42;
    let a = 99;
    let mut y = &x;
    let z = &mut y;
    *z = &a;
    println!("{}", *y);

will not print 99 after your optimization but 42

[simonvandel]

I added a test for this. It does not seem like it will misfire. I also added more clarifying comments on the matches. I have persuaded myself that since we are only applying this optimization on immutable references, we can't have a mutable reference at the same time, so nothing (besides asm) can break this optimization. My reasoning may be wrong though..

[simonvandel]

You are correct. Setting the lookback higher does indeed cause a miscompilation. I'll look into fixing it.

[oli-obk]

The general way to look for mutation is to define a visitor and visit the construct you want to analyze (Statement in your case) and implement https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/visit/trait.Visitor.html#method.visit_local to and check the PlaceContext if the local matches the local you want to check.

[simonvandel]

Ah, I saw your comment a tad too late. I already pushed a fix, but i'll see if it is cleaner/less adhoc to use the visitor. Thanks for the hint.

[simonvandel]

The latest commits now check that local_being_derefed is not mutated using a visitor. I have verified that setting the lookback to 10000 causes a misoptimization without the fix, and fixes it with the fix. I have set it back to to avoid bad runtime complexity. I'm not sure how best to represent that the fix is actually working. For now I separated it into two commits.

[oli-obk]

Thanks! Splitting it into individual commits made this great to review.

I also don't know how to test it. A bogus idea (Don't do this 😆): instead of 6 use 6 + mir_opt_level, then we can do -Zmir-opt-level=99999 on that test.

An alternative to having this magic number would be to implement the optimization as a feed forward (am I using words correctly here?) optimization. What I mean is that we don't go through every deref and look back, but we walk forward through each block and keep a set of "deref-optimizable" locals that we update as we go.

[oli-obk]

That said, I'm fine with merging this optimization as long as there's a tracking issue for exploring such a change to the optimization. I'm not sure whether that change is feasible in practice, but we should explore it

[simonvandel]

Cool. Yeah it should be possible to do this optimization without the lookback, such that a single statement is only visited once. I'll open a new pr for that.

Can we do a perf run on the current pr? I'm curious if this has any impact

[oli-obk]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 116283b910a7f8809260ba91cb9b9b647a8900a3 with merge 7d835e12834e0230779883b269523b01a940c2df...

[bors]

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

[rust-timer]

Queued 7d835e12834e0230779883b269523b01a940c2df with parent 10b3595, future comparison URL.

[rust-timer]

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

[oli-obk]

perf looks ok. Please squash so we get all the back and forth out of the timeline

[simonvandel]

Squashed and rebased on master

[oli-obk]

@bors r+

[bors]

📌 Commit 4dedb76 has been approved by oli-obk

[bors]

⌛ Testing commit 4dedb76 with merge 40fe3db7186781f895de3f6d860dbd5d570ec21c...

[bors]

💔 Test failed - checks-actions

[oli-obk]

Looks like you need to rebase and rebless again?

[simonvandel]

Rebased on master and added a new commit such that Add is always generated instead of CheckedAdd, which broke the test diff on noopt testruns.

[oli-obk]

@bors r+

[bors]

📌 Commit dfc469d has been approved by oli-obk

[bors]

⌛ Testing commit dfc469d with merge f47df31...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing f47df31 to master...

[eddyb]

@rust-lang/wg-mir-opt I think this would be better suited to constant-folding with a notion of "symbolic" values.
(not necessarily symbolic miri evaluation, but propagatable abstract values that represent runtime values which are effectively "constant" within one execution of the function)

But also I'm a bit worried this kind of optimization may break stacked borrows type assumptions if the indirection was "weakening" the semantics of the access, and making it direct "strengthens" it too much - though this may only be a problem with raw pointers.