RFR: JDK-8213489: GC/C2 abstraction for Compile::final_graph_reshaping()

[Vladimir Kozlov]

On 11/7/18 1:40 PM, Roman Kennke wrote:
> Hi Vladimir,
> 
>> Next
>>
>> +        assert(n->is_Mem(), "");
>> +        MemNode* mem  = (MemNode*)n;
>>
>> could be replaced with
>>
>> +        MemNode* mem  = n->as_Mem();
> 
> Right. I copied existing code from final_graph_reshaping. New changeset
> fixes it.

Would be nice if you also fix existing code which you copied.

> 
>> I don't see removal of moved ZGC code in
>> Compile::final_graph_reshaping_impl()
> 
> Oops, my bad. Fixed.
> 
>> Why you skip only assert and not whole switch() (or return) ? Do you
>> want to process some nodes twice: by GC first and then in main switch?
> 
> Yes. We have a case in Shenandoah where we want to process a CallNode
> Shenandoah-specific and then also want to verify/reshape generically.

What "verify/reshape generically" code you are referencing here? If opcode of Shenandoah's node is 
not listed in main switch only 'default:' code will be executed. And you are skipping code there in 
such case. I still do not get why you need to execute main switch when gc_handled is 'true'.

Do you want to process any nodes already listed in main switch? In such cases you would need to 
return 'false' and execute main switch - asserts will be valid in such case.

> 
>> May be pass nop (opcode) to final_graph_reshaping() too to avoid virtual
>> call Opcode() there. >
> OK
You did not replace Opcode() call in ZBarrierSetC2::final_graph_reshaping():

    switch (n->Opcode()) {

Thanks,
Vladimir

> 
> Incremental:
> http://cr.openjdk.java.net/~rkennke/JDK-8213489/webrev.01.diff/
> Full:
> http://cr.openjdk.java.net/~rkennke/JDK-8213489/webrev.01/
> 
> Better?
> 
> Thanks for reviewing!
> Roman
> 
>> Thanks,
>> Vladimir
>>
>> On 11/7/18 9:00 AM, Roman Kennke wrote:
>>> GCs might want to do something to nodes in
>>> Compile::final_graph_reshaping(). Let's put an abstraction in this place.
>>>
>>> The way I did it was to put a call into
>>> BSC2::final_graph_reshaping(Compile*, Node*) in front of the huge switch
>>> and let the caller know if the node was handled or not. This is
>>> subsequently checked in the default-branch: if GC handled it, the
>>> asserts are not checked. This should provide the exact same behaviour
>>> that we have now, only better and nicer.
>>>
>>> I also took the opportunity and moved the ZGC related parts there to
>>> ZBarrierSetC2.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8213489
>>> Webrev:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8213489/webrev.00/
>>>
>>> Testing: hotspot/jtreg:tier1 passes
>>>
>>> Thoughts? Reviews?
>>>
>>> Roman
>>>
>