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

[Vladimir Kozlov]

On 3/12/14 10:12 AM, Roland Westrelin wrote:
>
> Hi Vladimir,
>
> Thanks for taking a look at this.
>
>>> http://cr.openjdk.java.net/~roland/8031755/webrev.00/
>>>
>>> Most Type profiling points record whether a null reference was seen
>>> but type speculation doesn’t currently take advantage of it. With
>>> this change, not only is the type seen at a profiling point feed to
>>> type speculation but also whether a null pointer was seen or
>>> not. This is then used to optimize null checks.
>>>
>>> The _speculative field becomes:
>>> const TypePtr*   _speculative;
>>>
>>> so that if all we know from profiling is that a reference is not null:
>>> _speculative = TypePtr::NOT NULL;
>>>
>>> When a type is met with a TypePtr (NULL_PTR for instance), the meet
>>> must be applied to the speculative types as well. To keep the type
>>> system symmetric, the _speculative field had to be moved to class
>>> TypePtr. I also moved the _inline_depth there to keep all speculative
>>> stuff together but with the current code it could have stayed in
>>> class TypeOopPtr.
>>>
>>> Traps caused by a failed speculative null check are recorded with
>>> speculative traps, similarly to what is done for a failed class
>>> check.
>>
>> The idea is good, I think, but I need time to go through changes.
>>
>> What about changes in arguments.cpp and phaseX.cpp?
>
> In c2_globals.hpp
>
>    product(intx, MaxNodeLimit, 80000,                                        \
>            "Maximum number of nodes")                                        \
>
> So today, we're decreasing the number of nodes if incremental inlining is on.

Agree.

>
> In phaseX.cpp, the problem is that during an IGVN, some nodes that
> become disconnected from the graph are not removed from the IGVN's hash
> table. It's a bit ugly but maybe good enough for some verification code?

Yes, handling dead nodes is less then satisfactory in C2.

>
>> I don't understand why you changed higher_equal_speculative() in
>> parse2.cpp.
>
> I added a cleanup_speculative() to Type that removes the speculative
> type if it's useless (not an exact klass for instance). So:
>
> 1289             TypeNode* ccast = new (C) CheckCastPPNode(control(), obj, tboth);
> 1290             const Type* tcc = ccast->as_Type()->type();
> 1291             assert(tcc != obj_type && tcc->higher_equal(obj_type), "must improve");
>
> The speculative type may disappear and
> tcc->higher_equal_speculative(obj_type) is no longer true.

Okay.

>
>>> 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.
>
> I'll see if I can come up with a test case for this one.

Thanks,
Vladimir

>
> Roland.
>