Skip to content

Conversation

@ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Jan 11, 2018

Extends #4293, and inspired by #4351.

I will add tests later, just going to stand back and think about whether or not I'm missing something really obvious and any approach used here is wrong.

The additional branches match the following code:


  1. auto x = arr[n .. 0]
    Upper always in bounds if is proven to be 0 at compile time.

  1. auto x = arr[n .. $]
    Upper is always in bounds if is just a dollar var.

  1. auto x = arr[n .. $/m]
    Upper is using $. Check whether IntRange(upr).max < IntRange($).max.
    If m is a compile-time constant, this would affect the inferred range.
    For example, on 64bit:
// The length.max integer range is 18446744073709551615.

// These slices don't generate a bounds check.
[n .. $/4] => uprRange.max is 4611686018427387903
[n .. $%4] => uprRange.max is 3

// Bounds check still occurs here.
[n .. 4/$] => uprRange.max is 18446744073709551615

  1. auto x = arr[$/m .. $]
    Both lower and upper are using $. Check whether IntRange(lwr).max < IntRange(upr).max.
    If m is a compile-time constant, this would affect the inferred int range.
    For example, on 64bit:
// These slices don't generate a bounds check.
[$/4 .. $] => lwrRange.max is 4611686018427387903, uprRange.max is 18446744073709551615
[$%4 .. $/7] => lwrRange.max is 3, uprRange.max is 2635249153387078802

// Bounds check still occurs here.
[$*10/20 .. $*30/40] => lwrRange.max is 922337203685477580, uprRange.max is 461168601842738790

In the cases for 1,2,3, it sets upperIsInBounds = true.
In the case for 4, it sets lowerIsLessThanUpper = true.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@ibuclaw ibuclaw added the Review:WIP Work In Progress - not ready for review or pulling label Jan 11, 2018
@nordlow
Copy link
Contributor

nordlow commented Jan 11, 2018

Thanks for taking over this!

@ibuclaw
Copy link
Member Author

ibuclaw commented Jan 11, 2018

On some thought.

Case 1 and 2 are just a front-end optimization that the backend optimizer should be able to handle (gdc doesn't produce a bounds check during codegen).

Case 3 can't be validated using VRP alone, especially when the expression gets complex. Consider $*5/4 where arr.length = 4 (but we don't know that at compile time).

Case 4 can't be validated at all! If arr.length is 0 then lower is never less than upper.

So overall, while a nice idea that seems feasible. Unless everything is known at compile-time, the worst case scenario can only be assumed.

@nordlow ^^

On some closer inspection though, I see that there's a missed opportunity to correctly set these two fields correctly, as the lengthVar is not determined until the const folder runs.

Haven't checked if dmd does array bounds where it shouldn't because of this, but will sort that out later.

@ibuclaw ibuclaw closed this Jan 11, 2018
@ibuclaw ibuclaw deleted the vrpboundscheck branch January 11, 2018 10:34
@ibuclaw
Copy link
Member Author

ibuclaw commented Jan 11, 2018

Maybe if VRP detected also if an overflow could happen. Then we might have something workable here.

@ibuclaw
Copy link
Member Author

ibuclaw commented Jan 12, 2018

I'll have a look into whether an overflow flag could be added to the vrp code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review:Needs Work Review:WIP Work In Progress - not ready for review or pulling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants