Simple `Option` use, like in `checked_sub`, should optimize out fully in MIR

[scottmcm]

Today, this bit of code: https://rust.godbolt.org/z/EfE68Meax

pub fn saturating_sub_at_home(lhs: u32, rhs: u32) -> u32 {
    u32::checked_sub(lhs, rhs).unwrap_or(0)
}

Doesn't MIR-opt as well as it ought, because the Option isn't completely optimized out:

    bb2: {
        StorageLive(_5);
        _5 = SubUnchecked(copy _1, copy _2);
        _3 = Option::<u32>::Some(move _5);
        StorageDead(_5);
        StorageDead(_4);
        StorageLive(_6);
        _6 = move ((_3 as Some).0: u32);
        _0 = move _6;
        StorageDead(_6);
        goto -> bb3;
    }

Even though it's clearly unnecessary to build-then-unwrap the option here.

This isn't "just" a GVN bug, because when GVN sees it it's not all together in one BB like this, and IIRC the Option local is also not SSA at the time, so it's reasonable that GVN doesn't currently address it. Yay phase ordering :/

---

Once #138545 lands, there will be tests in the repo further demonstrating this, at https://github.com/search?q=repo%3Arust-lang%2Frust%20138544%20%20path%3Atests%2Fmir-opt&type=code

[saethlin]

Yay phase ordering :/

Is there a pass ordering that can make this optimizable?

[scottmcm]

Is there a pass ordering that can make this optimizable?

Run everything a second time? 🙃

I think the problem is that it's a mix of stuff, since it requires both value propagation and CFG changes, so there's no obvious "oh just do this first" solution, at least not that I can see.

Good news is that at least if #138582 works it'll remove the codegen-side cost of not unwrapping it in MIR.

[FractalFir]

I have been working on a pass that can remove those, as long as there is no other statement(besides NOP and storage) between the assigement and use.

It is a simple linear scan, so it should be pretty fast.

Right now, the biggest issue is posed by the StorageDead statement in the middle, since I need to check that the aggregate is not built from locals that "die" after that statement.

Consider MIR like this:

_2 = Option::<u32>::Some(move _1);
StorageDead(_1);
_3 =  move ((_2 as Some).0: u32);

Since local _1 is no longer alive in the second assigement, a transformation like this:

nop;
StorageDead(_1);
_3 =  move _1;

would be unsound. Going the other way posses the same problem with StorageLive's.

This is not a theoretical problem: this pattern occurs in float_to_exponential_common:

        _6 = Option::<usize>::Some(move _17);
        StorageDead(_17);
        StorageDead(_15);
        StorageDead(_14);
        StorageDead(_16);
        _7 = copy ((_6 as Some).0: usize);

I could go over all the locals referenced by the original aggregate, and check that the StorageDeads don't affect them, but that will not be enough.

Consider MIR like this:

_2 = &1
_3 = Option::<usize>::Some(move *(_2))
StorageDead(_1)
_4 =  move ((_3 as Some).0: u32);

Here, the operand move *(_2) is no longer valid, since it points to deallocated _1.

All of that makes getting rid of those simple uses of Option way harder.