RFR(XL): 8224675: Late GC barrier insertion for ZGC

[Nils Eliasson]

Hi Roland,

On 2019-05-29 09:48, Roland Westrelin wrote:
> Hi Nils,
>
>> Webrev: http://cr.openjdk.java.net/~neliasso/8224675/webrev.01/
> zBarrierSetC2.cpp: typo loadbarrers line 756
Fixed
> lcm.cpp:
>
> void PhaseCFG:: call_catch_cleanup(Block* block) {
>
> space after ::?
Fixed
>
> loopnode.cpp:
>
> Node *u
>
> I thought the usually recommended style was:
>
> Node* u
Yes.
> loopnode.cpp:
>
> Do we really need a new entry in the gc api for barrier_insertion()
> couldn't this go under optimize_loops()?

It could. There is only remove_range_check_casts in between right now. I 
choose to have it as its own for separation, in a similar way to how 
LoopOptsNone are used.


> memnode.hpp:
>
>   168   enum LoadBarrier {
>   169     UnProcessed     = 0,
>   170     RequireBarrier  = 1,
>   171     WeakBarrier     = 3,  // Inclusive with RequireBarrier
>   172     ExpandedBarrier = 4
>   173   };
>
> Shouldn't that be abstracted away through the gc api somehow?

Yes.

I would have preferred using the node-flags, but they are all taken 
unless we expand it to 32 bits, or overload the flags that are only used 
during codegen and later. That would require some verification of the 
flag use to catch any mistakes.

>
> zBarrierSetC2.cpp:
>
> typo (witch):
>
> 981     // For each load use, see witch catch projs dominates, create load clone lazily and reconnect
Fixed
>
> In fixup_uses_in_catch() wouldn't you need to deal with phis the way you
> do in call_catch_cleanup_one():
>
> 1019     // We found no single catchproj that dominated the use - The use is at a point after
> 1020     // where control flow from multiple catch projs have merged. We will have to create
> 1021     // phi nodes before the use and tie the output from the cloned loads together. It
> 1022     // can be a single phi or a number of chained phis, depending on control flow

No - it's a two step process.

There is a load hanging off a call with multiple catch projs. (There can 
be no other control-flow in between.) The load have uses, either (1) in 
the catch blocks, or (2) in some cases in the call block, hanging off 
the load.

fixup_uses_in_catch responsibility is to turn (2) into (1). It clones 
all uses of the load, in the call block, out to the catch blocks. This 
is done recursively. There can never be any phis here.


> Is there a chance that a load would be processed by
> fixup_uses_in_catch()? I see call_catch_cleanup_one() is where they are
> expected to be handled but you only go there if
> load->is_barrier_required() is true. So could you have a load for which
> is_barrier_required() is true have a use for which is_barrier_required()
> is not true and both of them be in the catch block?

In theory yes. If the load without a barrier is a use of the load with a 
barrier, it would be processed first. When the load with a barrier is 
processed, it would clone the load without a barrier, like any other node.

Another case is two loads that both require a barrier. One would be 
processed first, that one can't have the other one as a use, because the 
loads are processed in post order. And when the other load is processed, 
the first has already been cloned out.

Thanks for the feedback!

I'll get back with a new webrev.

// Nils

>
> Roland.