arrayref icon indicating copy to clipboard operation
arrayref copied to clipboard

avoid overflow cases to help eliminate bounds checks

Open oconnor663 opened this issue 6 years ago • 4 comments

Currently array_ref! takes a slice this way:

let slice = & $arr[offset..offset + $len];

If the length of slice is already known, the compiler should be able to skip bounds checks there. However, because of the possibility that offset + $len might overflow, the compiler sometimes has to be conservative. Switching to slightly different slicing arithmetic avoids this problem:

let slice = & $arr[offset..][..$len];

Here's an example of the second version successfully eliding bounds checks, where the first version does not (https://godbolt.org/z/Je4lRl):

fn get_array(input: &[u8], offset: usize) -> Option<&[u8; SIZE]> {
    if input.len() >= offset && input.len() - offset >= SIZE {
        Some(array_ref!(input, offset, SIZE))
    } else {
        None
    }
}

oconnor663 avatar Feb 15 '19 21:02 oconnor663

I think this might be a win in some practical cases. However, I've found at least one contrived case where it appears to be a loss. If we tweak the example above to use a constant-size input array, and then simplify the if-condition:

fn get_array(input: &[u8; 2*SIZE], offset: usize) -> Option<&[u8; SIZE]> {
    if offset <= SIZE {
        Some(array_ref!(input, offset, SIZE))
    } else {
        None
    }
}

In this case, it's the new approach that fails to elide bounds checks. I have no idea why. https://godbolt.org/z/dMqF2o

Have you experimented with this stuff before? Do you have any intuition about why this second example is failing to optimize?

oconnor663 avatar Feb 15 '19 21:02 oconnor663

I have not experimented with this at all, and have no idea.

droundy avatar Feb 16 '19 14:02 droundy

I've asked for advice on the subreddit, fingers crossed that someone there knows more: https://www.reddit.com/r/rust/comments/arcavd/why_does_rustc_eliminate_bounds_checks_in_one

oconnor663 avatar Feb 16 '19 19:02 oconnor663

Holy crap apparently they're working on a fix for this :)

https://reviews.llvm.org/D59916

oconnor663 avatar Mar 28 '19 17:03 oconnor663