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

[Vladimir Kozlov]

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.

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