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

[Vladimir Kozlov]

For me this version looks better.

Thanks,
Vladimir

On 11/21/18 12:04 PM, Roman Kennke wrote:
> 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
>>>>>>
>>>>
>>>
>