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

Tobias Hartmann tobias.hartmann at oracle.com
Wed Aug 6 07:38:56 UTC 2014


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
>>>>
>>



More information about the hotspot-compiler-dev mailing list