Request for reviews (M): 7059047: EA: can't find initializing store with several CheckCastPP

[Vladimir Kozlov]

Tom,

Could you look on this update? webrev.02 vs original webrev difference:

http://cr.openjdk.java.net/~kvn/7059047/webrev.update

Thanks,
Vladimir

Vladimir Kozlov wrote:
> Nothing was wrong with scalar_replaceable flag except it was not 
> propagated to referenced objects so ArgEscape state was used in such cases.
> 
> But I see what you are concern about. The escape state type should be 
> pure escape state and applicable optimizations should be controlled by 
> separate flag. OK, I returned back scalar_replaceable flag and 3 
> original escape states:
> 
> http://cr.openjdk.java.net/~kvn/7059047/webrev.02
> 
>  > Right, because ArgEscape means that we've lost track of identity 
> because of aliasing?
> 
> Correct.
> 
> Thanks,
> Vladimir
> 
> Tom Rodriguez wrote:
>> On Nov 1, 2011, at 1:59 PM, Vladimir Kozlov wrote:
>>
>>> Thank you, Tom
>>>
>>> Tom Rodriguez wrote:
>>>> I'm not sure I follow the distinction that's being made with 
>>>> ControlEscape, or ArgEscape for that matter.  They seem to be about 
>>>> what optimizations we can do on an object more than whether they are 
>>>> escaping or not, which makes the naming confusing.  Or am I 
>>>> misunderstand what's happening?  Is this how they would be translated?
>>>> NoEscape      == ScalarReplaceable
>>>> ControlEscape == NonEscaping
>>>> ArgEscape     == LocallyEscaping
>>>> GlobalEscape  == GlobalEcaping
>>> Your translation is correct. Yes, the states define optimizations we 
>>> can do with objects.
>>>
>>>> Would be it be possible to come up with more descriptive names for 
>>>> these states?
>>> How about your translation names?
>>
>> I'm happy with them of course but I wasn't sure they were accurate.  
>> Some comments explaining the distinction would be helpful too.
>>
>> In thinking about this some more, I'm wondering whether it's right to 
>> introduce this new state.  What was wrong with the old 
>> scalar_replaceable flag?  It feels to me like we have two distinct 
>> axes that we're trying to shove into a single number, which 
>> complicates the terminology for it.
>>
>>>> Why is has_non_escaping_obj assigned to twice?
>>>> // mark all nodes reachable from ArgEscape nodes
>>>> !   has_non_escaping_obj = propagate_escape_state(&cg_worklist, 
>>>> &worklist, PointsToNode::ArgEscape);
>>>> !   // mark all nodes reachable from ControlEscape nodes
>>>> !   has_non_escaping_obj = propagate_escape_state(&cg_worklist, 
>>>> &worklist, PointsToNode::ControlEscape);
>>>> Should the second one be |=?
>>> You are right. My bad.
>>>
>>>> Do we eliminate locking on ControlEscape objects?
>>> Yes, the same as ArgEscape.
>>>
>>> I will add code for 7105605 to fold CmpP for NoEscape and 
>>> ControlEscape objects. I don't think it will be safe to do for 
>>> ArgEscape.
>>
>> Right, because ArgEscape means that we've lost track of identity 
>> because of aliasing?
>>
>>> An other optimization I am thinking for ControlEscape objects is 
>>> eliminating loads from it as we do for ScalarReplaceable objects.
>>
>> That seems good, though that might change how we name the states.  
>> NoEscape means all optimizations and elimination of allocation, 
>> ControlEscape just means we can't eliminate the allocation but we can 
>> do other allocations.  Maybe the NoEscape shouldn't be renamed 
>> ScalarReplaceable then?
>>
>> tom
>>
>>> Thanks,
>>> Vladimir
>>>
>>>> tom
>>>> On Nov 1, 2011, at 11:41 AM, Vladimir Kozlov wrote:
>>>>> http://cr.openjdk.java.net/~kvn/7059047/webrev
>>>>>
>>>>> 7059047: EA: can't find initializing store with several CheckCastPP
>>>>>
>>>>> Added new escape state: ControlEscape. Use it instead of ArgEscape 
>>>>> and (NoEscape && !_scalar_replaceable) in cases where not escaped 
>>>>> objects can't be scalar replaced because of flow-insensitive 
>>>>> analysis limitations. It is preparation for  7105605 changes.
>>>>>
>>>>> Split adjust_escape_state() method into two. New find_init_values() 
>>>>> method is used to find fields initializing values for not escaped 
>>>>> allocations. It is called before deferred edges are removed since 
>>>>> it affects results. adjust_escape_state() now is called after all 
>>>>> deferred edges are removed to get correct results.
>>>>>
>>>>> Factored out escape state propagation code into new method 
>>>>> propagate_escape_state().
>>>>>
>>>>> Removed methods is_scalar_replaceable() and hidden_alias() which 
>>>>> are not used and corresponding fields in PointsToNode (which will 
>>>>> reduce used memory).
>>>>>
>>>>> Tested with CTW, jtreg, NSK, refworkload.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>