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

[Roman Kennke]

>> 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 accomplish:

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: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20181121/19549ff3/signature-0001.asc>