Request for Reviews (S): JDK-8003585 strength reduce or eliminate range checks for power-of-two sized arrays

[Vladimir Kozlov]

Thanks! Looks good.

Vladimir

On 1/19/16 7:22 AM, Roland Westrelin wrote:
> Thanks for taking another look at this, Vladimir.
>
>> I know it is duplication but CmpU creation should be under conditions otherwise you are creating and transforming dead node.
>>
>> +     Node* ncmp = phase->transform(new CmpUNode(cmp1, cmp2));
>> +     if (_test._test == BoolTest::le || _test._test == BoolTest::eq) {
>>
>> The test does not cover next conversions:
>>
>> +   // Change (arraylength <= 0) or (arraylength == 0)
>> +   //   into (arraylength u<= 0)
>> +   // Also change (arraylength != 0) into (arraylength u> 0)
>
> Here is a new webrev:
>
> http://cr.openjdk.java.net/~roland/8003585/webrev.02/
>
> Roland.
>
>>
>> Thanks,
>> Vladimir
>>
>> On 1/7/16 1:29 AM, Roland Westrelin wrote:
>>> Can I get a review for this?
>>>
>>> Roland.
>>>
>>>> On Oct 5, 2015, at 12:51 PM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
>>>>
>>>> Here is a new webrev:
>>>>
>>>> http://cr.openjdk.java.net/~roland/8003585/webrev.01/
>>>>
>>>> Roland.
>>>>
>>>>> On Oct 2, 2015, at 3:30 PM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
>>>>>
>>>>> Hi Chris,
>>>>>
>>>>>> Thanks for picking it up! It mostly looks good to me. (Not a Reviewer)
>>>>>
>>>>> Thanks for looking at this again.
>>>>>
>>>>>> What I really needed with my earlier webrev was some instructions as to what test to write -- since the Java corelibs can come across this optimization a lot (e.g. HashMap), I didn't have a good idea of what kind of test really needs to be written.
>>>>>>
>>>>>> A couple of issues with this webrev:
>>>>>>
>>>>>> 1. In subnode.cpp, line 1346:
>>>>>>
>>>>>> 1344     } else if (_test._test == BoolTest::lt &&
>>>>>> 1345                cmp2->Opcode() == Op_AddI &&
>>>>>> 1346                cmp2->in(2)->find_int_con(1)) {
>>>>>> 1347       bound = cmp2->in(1);
>>>>>> 1348     }
>>>>>>
>>>>>> I think it should be
>>>>>> cmp2->in(2)->find_int_con(0) == 1
>>>>>> instead, because the value passed into this function is actually for a "fallback when no int constant is found". Passing the expected value (1) to it defeats the purpose.
>>>>>
>>>>> You’re right. Thanks for spotting that.
>>>>>
>>>>>> jint find_int_con(jint value_if_unknown) const {
>>>>>>    const TypeInt* t = find_int_type();
>>>>>>    return (t != NULL && t->is_con()) ? t->get_con() : value_if_unknown;
>>>>>> }
>>>>>>
>>>>>> 2. Formattign nitpick: could you please trim the spaces before the new's on lines 1368, 1369 and 1387
>>>>>
>>>>> Sure.
>>>>>
>>>>> I’ll send an updated webrev.
>>>>>
>>>>> Roland.
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Kris (OpenJDK username: krismo)
>>>>>>
>>>>>> On Wed, Sep 30, 2015 at 1:34 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
>>>>>> I’m picking that one up. Here is a new webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~roland/8003585/webrev.00/
>>>>>>
>>>>>> The only change to c2 compared to the previous webrev is that ((x & m) u< m+1) is optimized the same way ((x & m) u<= m) is. Actually, I don’t think that C2 currently produces the ((x & m) u<= m) shape. The IfNode::fold_compares() logic produces the ((x & m) u< m+1) variant. I also added a test case to check the validity of the transformations and ran usual testing on the change.
>>>>>>
>>>>>> Roland.
>>>>
>>>
>