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

[Per Liden]

fixup_partial_loads() still appears in zHeap.hpp.

diff --git a/src/hotspot/share/gc/z/zHeap.hpp 
b/src/hotspot/share/gc/z/zHeap.hpp
--- a/src/hotspot/share/gc/z/zHeap.hpp
+++ b/src/hotspot/share/gc/z/zHeap.hpp
@@ -77,7 +77,6 @@
    void flip_to_remapped();

    void out_of_memory();
-  void fixup_partial_loads();

  public:
    static ZHeap* heap();


/Per

On 5/29/19 4:54 PM, Nils Eliasson wrote:
> 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.