Worse codegen for a get(a..a+N) slice range check on beta

[adrian17]

Code

Godbolt repro: https://godbolt.org/z/Yv3qYqq4W

Variant A: check a.., then check ..a+N.

pub fn write_u8(
    bytes: &mut [u8],
    buf: u8,
    offset: usize,
) -> Result<(), Box<dyn std::error::Error>> {
    let buf = buf.to_le_bytes();
    bytes
        .get_mut(offset..).and_then(|bytes| bytes.get_mut(..buf.len()))
        .ok_or_else(|| "RangeError: The specified range is invalid")?
        .copy_from_slice(&buf);
    Ok(())
}

Variant B: check a..a+N in one call.

pub fn write_u8(
    bytes: &mut [u8],
    buf: u8,
    offset: usize,
) -> Result<(), Box<dyn std::error::Error>> {
    let buf = buf.to_le_bytes();
    bytes
        .get_mut(offset..offset+buf.len())         // <- changed line
        .ok_or_else(|| "RangeError: The specified range is invalid")?
        .copy_from_slice(&buf);
    Ok(())
}

Note that N is a constant (1), as it's a size of an u8. Also note that that this example can be extended to other types of buf: u16, u32, [u8; 17] etc.

Results

• Variant A on stable 1.70: generates a single bytes.len() <= offset check, then sets the byte and returns Ok - perfect. (I wish the push/pop were only on the error path, but that's off topic).
• Variant B on stable 1.70: generates two branches. (I wish it was the same as A, but I guess there could be some overflow related differences.)
• Variant A on beta 1.71: same as on stable.
• Variant B on beta 1.71: seemingly, nonsense? The "happy path" is much longer and contains several conditional moves; also, the lea rdi, [rip + .L__unnamed_1] looks like it moved address calculation of the error string to the happy path.

This issue concerns the last one.

I did an ad-hoc microbenchmark that shows Variant B on Beta (and nightly) to become 2-3x slower, but I'm not sure if that benchmark needs attaching - to me it's clear from the disassembly that the happy path codegen is now worse.

[saethlin]

I think the change is that the MIR inliner now inlines Option::ok_or_else: https://godbolt.org/z/9Gz43cdvW If you add --emit=mir and look at the calls, that's the only difference from the increment of the inlining threshold that I can see. (yes I could do godbolt diff view but then I'd want 6 panes and that's a lot)

The follow-up question is why LLVM can't still optimize the MIR.

[Jules-Bertholet]

@rustbot label -regression-untriaged

[nikic]

llvm/llvm-project#63749

[adrian17]

Just to be sure, would this missing fold also solve the same issue with range sizes bigger than 1? Like: https://godbolt.org/z/KxbE97faE

[nikic]

No, that does not generalize. The previous issue was about the difference on 1.70.

The regression on 1.71 would be llvm/llvm-project#63756.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

[nikic]

Fixed by #114048, needs codegen test.