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

[Vladimir Kozlov]

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/

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
>