RFR(M): 8031754: Type speculation should favor profile data from outermost inlined method

[Christian Thalinger]

Sorry it took me so long.  This looks good.

On Feb 13, 2014, at 2:01 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

> 
> Can I have another review for this?
> 
> last webrev: http://cr.openjdk.java.net/~roland/8031754/webrev.01/
> 
> Roland.
> 
>>> I think it is good.
>>> 
>>> Can you add "Use" to flag name: UseInlineDepthForSpeculativeTypes? It is 
>>> more clear this way.
>> 
>> Sure. Thanks for the review, Vladimir.
>> 
>> Roland.
>> 
>> 
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>> On 2/10/14 3:40 PM, Roland Westrelin wrote:
>>>> Here is a new webrev:
>>>> 
>>>> http://cr.openjdk.java.net/~roland/8031754/webrev.01/
>>>> 
>>>> As suggested by Vladimir, I use a positive value for InlineDepthBottom and MAX for the meet of two depths. I also added asserts in remove_speculative. Whether this is used or not is under a command line option. There was also a missing argument to a call to TypeAryPtr::make() that went unnoticed because of default arguments.
>>>> 
>>>> Roland.
>>>> 
>>>> 
>>>> On Feb 6, 2014, at 7:29 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>> 
>>>>> On 2/6/14 10:13 AM, Roland Westrelin wrote:
>>>>>>>> Hi Vladimir,
>>>>>>>> 
>>>>>>>>>>> In general I am comfortable to have inline_depth as additional type's attribute. But only for speculative types when it make sense.
>>>>>>>>>>> 
>>>>>>>>>>> So why you keep _inline_depth in remove_speculative()?
>>>>>>>>>> 
>>>>>>>>>> Because it’s never used in practice with non speculative types (it should always be InlineDepthBottom for a non speculative type). The only place where it changes the behavior of compilation is in GraphKit::record_profile_for_speculation() where it’s used only for speculative types. Anyway, I can reset it to InlineDepthBottom in remove_speculative() if you like.
>>>>>>>>> 
>>>>>>>>> Yes, please, set to InlineDepthBottom. It will be consistent with types without speculative types.
>>>>>>>> 
>>>>>>>> Actually, that’s not possible. Because then, that code:
>>>>>>>> 
>>>>>>>> 2589     if (res_oopptr->remove_speculative() == res_oopptr->speculative()) {
>>>>>>>> 2590       return res_oopptr->remove_speculative();
>>>>>>>> 
>>>>>>>> in TypeOopPtr::xmeet()
>>>>>>>> 
>>>>>>>> doesn’t trigger if res_oopptr has an inline depth of InlineDepthTop. remove_speculative() transforms it to InlineDepthBottom. And the type system is no longer symmetric.
>>>>>>> 
>>>>>>> Do you know why it is top? The default value is InlineDepthBottom, how it is converted to top?
>>>>>> 
>>>>>> It’s top because it’s a join and for a join the dual of the _inline_depth is used.
>>>>> 
>>>>> Sorry, I misunderstood the code in remove_speculative(). I thought you replacing type's _inline_depth with one from speculative type but you simple preserving original _inline_depth of this type. The original code you had is good.
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>>> 
>>>>>> Roland.
>>>>>> 
>>>>>>> 
>>>>>>>> 
>>>>>>>> Instead shouldn’t I assert in remove_speculative() that if _speculative is non NULL then _inline_depth is InlineDepthTop or InlineDepthBottom?
>>>>>>> 
>>>>>>> To have assert is good but we need to understand why in such case inline_depth could be top.
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>> 
>>>>>>>> 
>>>>>>>> Roland.
>>>>