RFR(L): 8031755: Type speculation should be used to optimize explicit null checks

[Igor Veresov]

Seems alright.

igor


On Mar 28, 2014, at 8:52 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

> I need another review for this. Anyone?
> 
> Roland.
> 
> On Mar 26, 2014, at 9:55 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
> 
>> Thanks for the review, Vladimir.
>> 
>> Roland.
>> 
>> 
>> On Mar 26, 2014, at 1:43 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>> 
>>> It is good.
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>> On 3/25/14 7:53 AM, Roland Westrelin wrote:
>>>> Thanks for reviewing this, Vladimir!
>>>> 
>>>>> Looks good. I see next expression several times:
>>>>> 
>>>>> speculative ? Deoptimization::Reason_speculate_null_check : Deoptimization::Reason_null_check
>>>>> 
>>>>> Can you put it into method Deoptimization::reason_null_check(speculative)?
>>>> 
>>>> Here it is:
>>>> 
>>>> http://cr.openjdk.java.net/~roland/8031755/webrev.02/
>>>> 
>>>> I did the same for Reason_speculate_class_check & Deoptimization::Reason_class_check.
>>>> 
>>>> Roland.
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>> On 3/20/14 4:52 AM, Roland Westrelin wrote:
>>>>>>>> I made the following change:
>>>>>>>> 
>>>>>>>> *** 3561,3571 ****
>>>>>>>> 
>>>>>>>> 
>>>>>>>>     // Since klasses are different, we require a LCA in the Java
>>>>>>>>     // class hierarchy - which means we have to fall to at least NotNull.
>>>>>>>>     if( ptr == TopPTR || ptr == AnyNull || ptr == Constant )
>>>>>>>>       ptr = NotNull;
>>>>>>>> 
>>>>>>>> -     instance_id = InstanceBot;
>>>>>>>> 
>>>>>>>> 
>>>>>>>>     // Now we find the LCA of Java classes
>>>>>>>>     ciKlass* k = this_klass->least_common_ancestor(tinst_klass);
>>>>>>>>     return make(ptr, k, false, NULL, off, instance_id, speculative, depth);
>>>>>>>>   } // End of case InstPtr
>>>>>>>> 
>>>>>>>> --- 3706,3715 ——
>>>>>>>> 
>>>>>>>> because I hit a type not symmetric failure that I think it causes:
>>>>>>>> 
>>>>>>>> === Meet Not Symmetric ===
>>>>>>>> t   =                   javax/management/openmbean/OpenType:AnyNull * (inline_depth=-2)
>>>>>>>> this=                   javax/management/openmbean/ArrayType:TopPTR *,iid=top (inline_depth=InlineDepthTop)
>>>>>>>> mt=(t meet this)=       javax/management/openmbean/ArrayType:AnyNull * (inline_depth=-2)
>>>>>>>> t_dual=                 javax/management/openmbean/OpenType:NotNull *,iid=top (inline_depth=2)
>>>>>>>> this_dual=              javax/management/openmbean/ArrayType *
>>>>>>>> mt_dual=                javax/management/openmbean/ArrayType:NotNull *,iid=top (inline_depth=2)
>>>>>>>> mt_dual meet t_dual=    javax/management/openmbean/OpenType:NotNull * (inline_depth=2)
>>>>>>>> mt_dual meet this_dual= javax/management/openmbean/ArrayType *
>>>>>>> 
>>>>>>> The question is why OpenType:AnyNull does not have iid=top. It is in upper type lattice.
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~roland/8031755/TestMeetInstanceId.java
>>>>>> 
>>>>>> triggers this. But while working on reproducing it, I noticed a bug in the previous webrev:
>>>>>> 
>>>>>> if (!speculative_maybe_null) {
>>>>>> 
>>>>>> in TypePtr::would_improve_ptr() should have been:
>>>>>> 
>>>>>> if (!speculative_maybe_null()) {
>>>>>> 
>>>>>> with that fixed, the bug doesn’t reproduce. I also think we should drop the speculative type, if it is above the center line. So here is a new webrev with:
>>>>>> instance_id = InstanceBot;
>>>>>> back where it was.
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~roland/8031755/webrev.01/
>>>>>> 
>>>>>> Roland.
>>>>>> 
>>>> 
>> 
>