RFR: JDK-8213615: GC/C2 abstraction for escape analysis

[Vladimir Kozlov]

On 11/14/18 3:14 PM, Roman Kennke wrote:
>>>> Why add_final_edges_unsafe_access() does not include 'if (opcode ==
>>>> Op_GetAndSetP' code? You broke sequence where (adr_type == NULL) should
>>>> be checked first.
>>>
>>> Hmm, right. The idea was that Sheanndoah would need to call this for its
>>> own set of CAS nodes, we'd handle the first part separately anyway, and
>>> would therefore not be needed in general path:
>>>
>>>    if (opcode == Op_GetAndSetP || opcode == Op_GetAndSetN ||
>>>         opcode == Op_CompareAndExchangeN || opcode ==
>>> Op_CompareAndExchangeP) {
>>>       add_local_var_and_edge(n, PointsToNode::NoEscape, adr, NULL);
>>>     }
>>>
>>> However, since it's guarded by the if (opcode== ..) it doesn't hurt to
>>> leave it there. I reinstated the original sequence.
>>
>> You forgot to remove this code from main switch after copying it to
>> add_final_edges_unsafe_access().
> 
> Yikes! I should not work late in the evening...
> 
> Updated:
> Incremental:
> http://cr.openjdk.java.net/~rkennke/JDK-8213615/webrev.03.diff/
> Full:
> http://cr.openjdk.java.net/~rkennke/JDK-8213615/webrev.03/
> 
> 
> Good now?

Yes, it is good. Please, run testing before push.

Thanks,
Vladimir

> 
> Thanks for reviewing, Vladimir!
> 
> Roman
> 
>>>> Also fail check is wrong. Should be:
>>>>
>>>>          if (add_final_edges_unsafe_access(n, opcode)) {
>>>>            break;
>>>>          }
>>>>          ELSE_FAIL("Op_StoreP");
>>>
>>> Right. Fixed.
>>>
>>>> Why you moved record_for_optimizer() to .cpp file?
>>>
>>> This is because BarrierSetC2 impls would now include escape.hpp but not
>>> necessarily phaseX.hpp, or at least not necessarily phaseX.hpp before
>>> escape.hpp. And I did not want to drag this include into escape.hpp
>>> (which would be correct if the method is using a PhaseIGVN method).
>>> Instead, I moved the body into escape.cpp and we have the correct
>>> include there already.
>>
>> Okay. Good to know.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> Updated webrevs:
>>> Incremental:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8213615/webrev.02.diff/
>>> Full:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8213615/webrev.02/
>>>
>>> Better?
>>>
>>> Thanks a lot for reviewing!
>>> Roman
>>>
>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 11/13/18 9:05 AM, Roman Kennke wrote:
>>>>> Sure no problem. Thank you!
>>>>>
>>>>> Roman
>>>>>
>>>>>
>>>>>> I have to review it in details. Give me some time.
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 11/13/18 4:24 AM, Roman Kennke wrote:
>>>>>>> Thanks Roland for the review!
>>>>>>>
>>>>>>> Vladimir: do you want to take a deeper look too? Or can I consider
>>>>>>> your
>>>>>>> 'I like the proposal from JIT POV' as 'reviewed' ? ;-)
>>>>>>>
>>>>>>> Roman
>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8213615/webrev.01/
>>>>>>>>
>>>>>>>> Looks ok to me.
>>>>>>>>
>>>>>>>> Roland.
>>>>>>>>
>>>>>>>
>>>>>
>>>
>