MIR InstCombine introduces copies of mutable borrows

[scottmcm]

https://play.rust-lang.org/?gist=9f782298cc5b0f6cd557baef9e443096&version=nightly

fn blah(mut x: String) {
    do_something(&mut x);
}

generates the following MIR:

    let mut _0: ();                      // return pointer
    let mut _2: ();
    let mut _3: &mut std::string::String;
    let mut _4: &mut std::string::String;

    bb0: {                              
        StorageLive(_3);                 // scope 0 at src/main.rs:5:18: 5:24
        StorageLive(_4);                 // scope 0 at src/main.rs:5:18: 5:24
        _4 = &mut _1;                    // scope 0 at src/main.rs:5:18: 5:24
        _3 = _4;                         // scope 0 at src/main.rs:5:18: 5:24
        _2 = const do_something(move _3) -> [return: bb1, unwind: bb3]; // scope 0 at src/main.rs:5:5: 5:25
    }

Which -- though we don't run it again after optimizations -- wouldn't pass TypeckMir since it's a copy of a &mut String.

That came from InstCombine on the following

        _3 = &mut (*_4);                 // scope 0 at lib.rs:33:18: 33:24

This may or may not be a problem, but I wanted to file it in case it impacts any of the assumptions desired from the Copy/Move split on Operand.

Edit: I suppose this instcombine has long potentially changed the type of the operand, since it doesn't check mutability of the outer & or the borrow being *d.

cc @eddyb

[eddyb]

cc @nikomatsakis

[oli-obk]

Is the solution to change &mut *_4 to move _4 and &*_4 to just _4?

[eddyb]

@oli-obk You really don't want move semantics. I think copying is correct at those lower levels, even if the type doesn't implement Copy (you can consider it "an unsafe copy").

[nikomatsakis]

I personally consider this at least potentially OK. My view has been that 'the thing we call MIR' is really at least two IRs -- maybe more! -- that are represented using one set of data structures, with different invariants:

When first constructed, we have MIR proper.

Then, after drop elaboration has completed, we have a lower-level IR. Plausibly, in this IR, the aliasing rules are weaker.

That said, we may want to keep the stricter rules, so that we can optimize more on that basis!

Regardless of all of that, it would be great to document the assumptions at each point (e.g., in the rustc-guide).

[scottmcm]

This is now also fixed by #72820, which removed this fold in InstCombine