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

[Tobias Hartmann]

Thank you, Vladimir.

Best,
Tobias

On 06.08.2014 18:44, Vladimir Kozlov wrote:
> Good.
>
> Thanks,
> Vladimir
>
> On 8/6/14 12:38 AM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> thanks for the review.
>>
>> On 05.08.2014 18:47, Vladimir Kozlov wrote:
>>> 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.
>>
>> Done. New webrev:
>>
>> http://cr.openjdk.java.net/~thartmann/8043284/webrev.03/
>>
>> Thanks,
>> Tobias
>>
>>>
>>> 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
>>>>>>
>>>>
>>