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

[Roman Kennke]

Thanks, Vladimir!

Yes, will push it through jdk/submit first. (as always)

Roman

> 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.
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181115/b66c0710/signature.asc>