RFR: JDK-8214055: GC/C2 abstraction for phaseX

[Roman Kennke]

Hi Vladimir,

> Got it. This is unfortunate. In such case I would suggest to leave ZGC
> code as it is (but fix worklist) and have separate interface for
> Shenandoah which do nothing for ZGC.

Well I disagree here. I think it's burying GC-specific code path there
in shared code, and it doesn't even say 'Look: this is ZGC specific'. It
could be argued that other GC which would use ZGC-style load-barriers
would also want to do this (I guess this is the intention), but the
truth is, we don't know that now.

That being said, I'd leave this decision to ZGC devs. With the proposed
interface would allow to move the code into ZGC if considered feasible,
so it could be done in follow-up patch.

http://cr.openjdk.java.net/~rkennke/JDK-8214055/webrev.01/

How's that? Good to go?

Roman

> Thanks,
> Vladimir
> 
> On 11/21/18 11:18 AM, Roman Kennke wrote:
>>
>>
>>>> Why you need 2 separate functions which do almost the same?
>>>
>>> The same can be asked about the enclosing methods there
>>> (PhaseIterGVN::add_users_to_worklish() and PhaseCPP::analyze()). I guess
>>> the outer semantics should equal the GC interface semantics.
>>>
>>>> And why
>>>> iterate over all nodes again?
>>>> Can you have just one method which you call inside both loops?
>>>
>>> You mean for ZGC. We probably could. But for Shenandoah we have a
>>> completely different block of things to accomplish, so I went  for the
>>> more general interface. Yeah, it means to 'duplicate' the loop, but it
>>> seemed the lesser evil to me.
>>
>> Oh and btw, the semantics for the two calls are indeed different for
>> Shenandoah. Check this out to understand what I'm trying to accomplis.
>>
>> https://builds.shipilev.net/patch-openjdk-shenandoah-jdk-only-shared/latest/src/hotspot/share/opto/phaseX.cpp.udiff.html
>>
>>
>> Roman
>>
>>> Thanks,
>>> Roman
>>>
>>>> void add_users_to_worklist(Unique_Node_List& worklist, Node* u) {
>>>>    // Search for load barriers behind the load
>>>>    for (DUIterator_Fast i3max, i3 = u->fast_outs(i3max); i3 < i3max;
>>>> i3++) {
>>>>      Node* b = u->fast_out(i3);
>>>>      if (is_gc_barrier_node(b)) {
>>>>        worklist.push(b);
>>>>      }
>>>>    }
>>>> }
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 11/21/18 2:53 AM, Roman Kennke wrote:
>>>>> There are (Z)GC specific changes in
>>>>> PhaseIterGVN::add_users_to_worklist() and PhaseCCP::analyze(), and
>>>>> Shenandoah needs to add some stuff there too. Let's put in an
>>>>> abstraction for this.
>>>>>
>>>>> It should be noted that the ZGC path is now unwoven from the general
>>>>> path, at the expense of iterating the outputs 2x. I don't expect
>>>>> this to
>>>>> be a problem.
>>>>>
>>>>> The way it was before I did not like much, because it built some
>>>>> fairly
>>>>> ZGC specific implicit knowledge (has_load_barriers()) into the shared
>>>>> path, the way it's done now it seems more explicitely ZGC path. It
>>>>> could
>>>>> be argued that a future GC with load-barriers would need this too, but
>>>>> we don't know this yet. It could just the same be the case that a
>>>>> future
>>>>> GC has load-barriers, but needs slightly different handling there. I'd
>>>>> even say that this type of implicit knowledge is worse than simply
>>>>> guarding a path with #if INCLUDE_ZGC or similar, because the latter
>>>>> actually screams "THIS IS GC SPECIFIC" while the current implicit way
>>>>> doesn't.
>>>>>
>>>>> Also note that this includes a subtle bugfix: in the
>>>>> PhaseCPP::analyze()
>>>>> path, the ZGC version pushed to _worklist, but every other code there
>>>>> uses the local worklist instead.
>>>>>
>>>>> Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8214055
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8214055/webrev.00/
>>>>>
>>>>> Testing: tier1 ok
>>>>>
>>>>> Can I please get reviews?
>>>>>
>>>>> Thanks, Roman
>>>>>
>>>
>>

-------------- 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/20181121/d48eb6eb/signature.asc>