opt-level=z fails to remove bounds checks/panics even if every other opt-level succeeds

[thomcc]

If compiling with -Copt-level=z we fail to remove bounds checks and panicking branches in code like this (godbolt https://godbolt.org/z/YsoMszxPe):

pub fn read_up_to_8(buf: &[u8]) -> u64 {
    if buf.len() < 4 {
        // actual instance has more code.
        return 0;
    }
    let lo = u32::from_le_bytes(buf[..4].try_into().unwrap()) as u64;
    let hi = u32::from_le_bytes(buf[buf.len() - 4..][..4].try_into().unwrap()) as u64;
    lo | (hi << 8 * (buf.len() as u64 - 4))
}

Unfortunately, while -Copt-levels=[123s] all manage to remove the panic branches, -Copt-level=z does not, leading to a lot more code and significantly worse performance.

Note that we do manage to remove it in cases where the function does less stuff -- it's removed entirely in simple examples that do nothing other than load the value from a slice. However, in real code, (perhaps after a layer of inlining) you tend to have a couple of these in a function, and this issue seems to crop up again¹.

Given that this pattern is more or less the recommended "safe" way to read a u32 out of a &[u8], this seems unfortunate to me.

Footnotes

1. Although not 100% reliably -- having trouble reproducing on godbolt, but it seems like it's happening in real-world code. ↩

[scottmcm]

I see two problems here:

1. It looks like the unwrap isn't inlined, and the check part of an unwrap probably always should be -- looking at the discriminant of a Result/Option is trivial, and would probably optimize out enough to be worth doing even in -Oz, with just the cold part of triggering the panic outlined.

2. We need better methods for getting arrays out of slices. For example, writing this with first_chunk and last_chunk (https://godbolt.org/z/9qx3o9b76) from Tracking Issue for slice_first_last_chunk feature (slice::{split_,}{first,last}_chunk{,_mut}) #111774, instead of the manual length check, makes it work.

[thomcc]

Is there a reason to assume s.first_chunk::<4>().unwrap() would optimize better than s.try_into().unwrap()? It isn't clear to me why this would be.

Edit: Ah, there's a godbolt link. Note that this kind of indexing is extremely common in certain code (including commonly used functions like hash implementations), and isn't always as simple as the example (which is just first/last). It would be better if we could just do better in opt-level=z. Even opt-level=1 gets this right.

[scottmcm]

If there's an unwrap in both, then no, I'm not super-confident in it. But if it can be rephrased to not need the unwrap at all -- replacing the length check that needs deduping with the if let Some( -- then yes, I'd expect it to work better.

Have you perchance looked at -Os? I don't really know what the difference is between s and z for us or for LLVM, but it also wouldn't surprise me if z is incredibly unwilling to inline stuff.

[thomcc]

Yes -Os is fine (I mentioned as much above). The only two opt-levels with issues here are 0 (which is expected) and z (which is not).

Note that in practice using if-let is viable for this version of the function, but it usually is not viable for this pattern.

[scottmcm]

Result::unwrap is now inline(always), which appears to resolve this. It internally uses a call to std::result::unwrap_failed, which is not inlined, to keep from exploding the size from inlining the discriminant check.