8066103: C2's range check smearing allows out of bound array accesses

[Roland Westrelin]

> That's good; let's do it that way!
> 
> Final suggestion (really):  Add some more test cases, containing duplicate array indexes, to exercise the "local" logic. 

Thanks John. I’ll add some test cases. Do you want to see another webrev with test cases?

Roland.

> 
> – John
> 
>> On Dec 8, 2014, at 5:30 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
>> 
>> Hi John,
>> 
>> Thanks for the suggestions. They sound very reasonable to me. Here is a new webrev:
>> 
>> http://cr.openjdk.java.net/~roland/8066103/webrev.03/
>> 
>> In the patch below:
>> 
>>> Or better yet a purely local cutout.  (This sketch has a bad goto; the value of 'prev_dom' carried by the goto is what I called 'most_recent_prev_check' above.)
>>> 
>>> diff --git a/src/share/vm/opto/ifnode.cpp b/src/share/vm/opto/ifnode.cpp
>>> --- a/src/share/vm/opto/ifnode.cpp
>>> +++ b/src/share/vm/opto/ifnode.cpp
>>> @@ -876,6 +876,7 @@
>>>   // Low and high offsets seen so far
>>>   jint off_lo = offset1;
>>>   jint off_hi = offset1;
>>> +    int nb_checks = 0;
>>> 
>>>   // Scan for the top 2 checks and collect range of offsets
>>>   for( int dist = 0; dist < 999; dist++ ) { // Range-Check scan limit
>>> @@ -890,6 +891,15 @@
>>>       // the same array bounds.
>>>       if( flip2 == flip1 && range2 == range1 && index2 == index1 &&
>>>           dom->outcnt() == 2 ) {
>>> +          if (nb_checks == 0 && dom->in(1) == in(1)) {
>>> +            // Found an immediately dominating test at the same offset.
>>> +            // This kind of back-to-back test can be eliminated locally,
>>> +            // and there is no need to search further for dominating tests.
>>> +            assert(offset2 == offset1, "");
>>> +            prev_dom = dom;
>>> +            goto got_prev_dom;  // FIXME: unstructured control flow
>>> +          }
>>> +          nb_checks++;
>>>         // Gather expanded bounds
>>>         off_lo = MIN2(off_lo,offset2);
>>>         off_hi = MAX2(off_hi,offset2);
>>> @@ -985,6 +995,7 @@
>>> 
>>> } // End of Else scan for an equivalent test
>>> 
>>> + got_prev_dom:  // FIXME: unstructured control flow
>>> // Hit!  Remove this IF
>>> #ifndef PRODUCT
>>> if( TraceIterativeGVN ) {
>> 
>> I don’t think you need prev_dom. prev_dom is the projection, dom is the If node. IfNode::dominated_by needs the projection as input.
>> 
>> Roland.
>>