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

[Vladimir Kozlov]

Thank you, Tom

Vladimir

Tom Rodriguez wrote:
> On Nov 7, 2011, at 12:37 PM, Vladimir Kozlov wrote:
> 
>> Tom,
>>
>> Could you look on this update? webrev.02 vs original webrev difference:
>>
>> http://cr.openjdk.java.net/~kvn/7059047/webrev.update
> 
> Yes, that looks good.
> 
> tom
> 
> 
>> 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
>