[9] RFR(S): 8043284: Optimize signed integer comparison

[Vladimir Kozlov]

On 8/5/14 12:55 AM, Tobias Hartmann wrote:
> Hi Vladimir,
>
> thanks for the review.
>
>> The second condition is still in webrev:
>>
>> >> sub_tr1 == TypeInt::CC_GT && sub_tr2 == TypeInt::CC_LT
>
> Sorry, I forgot to update the webrev after removing the condition.
>
> As Rickard suggested, I moved the code into a separate method called 'fold_cmpI'. If you think that the method has too
> many arguments, we could reduce them to '(PhaseGVN* phase, Node* cmp)' and re-compute the other values locally.

I can't find Rickard's review. I am fine with this since BoolNode::Ideal() become too large.
cmp2 is not used, don't need to pass it. Declare cmp argument as SubNode* and pass cmp->as_Sub() so you don't need to do 
it inside.

Thanks,
Vladimir

>
> New webrev: http://cr.openjdk.java.net/~thartmann/8043284/webrev.02/
>
> Thanks,
> Tobias
>
>>
>> Thanks,
>> Vladimir
>>
>> On 8/4/14 7:41 AM, Tobias Hartmann wrote:
>>> Hi Vladimir,
>>>
>>> thanks for the review. Please see comments inline.
>>>
>>> On 31.07.2014 18:43, Vladimir Kozlov wrote:
>>>> You missed cop == Op_CmpI check.
>>>
>>> Thanks, I added the check.
>>>
>>>> And add checks for r0 and r1 != TypeInt::INT (no bottom type) in addition to != NULL.
>>>
>>> Done.
>>>
>>>> You don't need t2 because there is already cmp2_type. Check cmp2_type for != TypeInt::INT too.
>>>
>>> Done.
>>>
>>>> Use as_Sub() (which is cast) instead of isa_Sub() because there is is_Sub() check at the beginning already.
>>>
>>> Done.
>>>
>>>> I don't see how second condition could happen because tr1 < tr2:
>>>>
>>>> sub_tr1 == TypeInt::CC_GT && sub_tr2 == TypeInt::CC_LT
>>>
>>> Of course, you are right. I removed the second condition.
>>>
>>>> Instead of ConINode::make() you can use phase->intcon().
>>>
>>> I first used phase->intcon() but this method may return a cached ConINode and hit the assert "Idealize should return new
>>> nodes, use Identity to return old nodes" in 'PhaseIterGVN::transform_old()'. That's why I directly create the ConINode.
>>>
>>> New webrev: http://cr.openjdk.java.net/~thartmann/8043284/webrev.01/
>>>
>>> Thanks,
>>> Tobias
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 7/31/14 5:35 AM, Tobias Hartmann wrote:
>>>>> Hi,
>>>>>
>>>>> please review the following patch that fixes JDK-8043284.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8043284
>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8043284/webrev.00/
>>>>>
>>>>> == Problem ==
>>>>> Similar to the previous implementation of unsigned integer comparison (see JDK- 8042786), signed integer comparison
>>>>> does
>>>>> only consider the general integer range [MinInt, MaxInt] instead of two ranges when comparing the result of an
>>>>> overflowing AddI/SubI. However, there are situations where one can prove that both resulting ranges are unequal to the
>>>>> tested value (see 'TestIntegerComparison::testSigned' in the webrev). In this case the comparison can be optimized.
>>>>>
>>>>> == Solution ==
>>>>> Similar to the fix for JDK-8042786 we compute both type ranges if the add/sub overflows and compare them to the tested
>>>>> value. If we can prove that the value is always unequal, we replace the BoolNode by true or false (depending on the
>>>>> test).
>>>>> A jtreg test is added that triggers both the unsigned as well as the signed optimization.
>>>>>
>>>>> == Testing ==
>>>>> - Jtreg test (TestIntegerComparison)
>>>>> - JPRT
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>
>