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

[Nils Eliasson]

Latest webrev uploaded with all the fixes you suggested:

http://cr.openjdk.java.net/~neliasso/8224675/webrev.03/

Regards,

Nils


On 2019-05-29 14:18, Nils Eliasson wrote:
> 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.