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

Roman Kennke rkennke at redhat.com
Wed Nov 14 20:59:51 UTC 2018


Hi Vladimir,

> G1 and ZGC code refactoring matches previous code.
> add_to_congraph_unsafe_access() is also good.

OK.

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

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

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: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20181114/b25070c9/signature.asc>


More information about the hotspot-gc-dev mailing list