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

[Roman Kennke]

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/ef166b04/signature.asc>