Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid overflow cases to help eliminate bounds checks #16

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oconnor663
Copy link
Contributor

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
    }
}

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
Copy link
Contributor Author

oconnor663 commented Feb 15, 2019

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 oconnor663 changed the title avoid overflow case help eliminate bounds checks avoid overflow cases to help eliminate bounds checks Feb 15, 2019
@droundy
Copy link
Owner

droundy commented Feb 16, 2019

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

@oconnor663
Copy link
Contributor Author

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
Copy link
Contributor Author

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

https://reviews.llvm.org/D59916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants