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

[Vladimir Kozlov]

Good.

Vladimir

On 11/8/18 11:43 AM, Roman Kennke wrote:
> 
> 
>> You missed change (n->Opcode() -> opcode) again :)
>>
>> +bool ZBarrierSetC2::final_graph_reshaping(Compile* compile, Node* n,
>> uint opcode) const {
>> +  bool handled;
>> +  switch (n->Opcode()) {
>>
>>> http://cr.openjdk.java.net/~rkennke/JDK-8213489/webrev.02/
> 
> Duh. (hopefully) final webrev:
> http://cr.openjdk.java.net/~rkennke/JDK-8213489/webrev.03/
> 
> Thanks for reviewing!
> 
> Roman
> 
> 
>> The rest looks good.
>>
>> Thanks,
>> Vladimir
>>
>> On 11/8/18 10:50 AM, Roman Kennke wrote:
>>> Hi Vladimir,
>>>
>>>>>>
>>>>>> +        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.
>>>
>>> Right. See below for new changeset.
>>>
>>>>>> 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'.
>>>
>>> We want to handle CallLeaf to remove a call to a (useless) SATB barrier
>>> for which the corresponding store has been eliminated. G1 does something
>>> similar, but can figure it out coming from the post-barrier, which has a
>>> link to the pre-barrier. We need to do it this way. After we've done
>>> that, we still want the rest of CallLeaf processing to apply.
>>>
>>> See:
>>> https://builds.shipilev.net/patch-openjdk-shenandoah-jdk-only-shared/latest/src/hotspot/share/opto/compile.cpp.sdiff.html
>>>
>>>
>>> around line 2861.
>>>
>>>> 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.
>>>
>>> Right, that would be a more intuitive way to do it. My original idea was
>>> to avoid touching the default branch if GC handled the node so that we
>>> don't accidentally trip one of the asserts there, but we can use the
>>> same flag to avoid the switch altogether.
>>>
>>> In order to make this sane, I moved the main switch into its own method.
>>>
>>> Updated webrev:
>>> Incremental:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8213489/webrev.02.diff/
>>> Full:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8213489/webrev.02/
>>>
>>> Roman
>>>
>