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

[Vladimir Kozlov]

Why you need 2 separate functions which do almost the same? And why iterate over all nodes again?
Can you have just one method which you call inside both loops?

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
>