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

[Roman Kennke]

> 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
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181108/15e1f3b5/signature.asc>