RFR: 8304049: C2 can not merge trivial Ifs due to CastII

Roland Westrelin roland at openjdk.org
Wed Mar 15 12:16:20 UTC 2023


On Wed, 15 Mar 2023 10:37:03 GMT, Yi Yang <yyang at openjdk.org> wrote:

> Hi can I have a review for this patch? C2 can not apply Split If for the attached trivial case. PhiNode::Ideal removes itself by unique_input but introduces a new CastII
> 
> https://github.com/openjdk/jdk/blob/e3777b0c49abb9cc1925f4044392afadf3adef61/src/hotspot/share/opto/cfgnode.cpp#L1470-L1474
> 
> https://github.com/openjdk/jdk/blob/e3777b0c49abb9cc1925f4044392afadf3adef61/src/hotspot/share/opto/cfgnode.cpp#L2078-L2079
> 
> Therefore we have two Cmp, which is not identical for split_if.
> 
> ![image](https://user-images.githubusercontent.com/5010047/225285449-b41dc939-1d3f-45f3-b6d6-a9b9445c2f6a.png)
> (Fig1. Phi#41 is removed during ideal, create CastII#58 then)
> 
> ![image](https://user-images.githubusercontent.com/5010047/225285493-30471f1c-97b0-452b-9218-3b5f09f09859.png)
> (Fig2. CmpI#42 and CmpI#23 are different comparisons, they are not identical_backtoback_ifs )
> 
> This patch adds Cmp identity to find existing Cmp node, i.e. Cmp#42 is identity to Cmp#23
> 
> 
>   public static void test5(int a, int b){
>  
>     if( b!=0) {
>       int_field = 35;
>     } else {
>       int_field =222;
>     }
> 
>     if( b!=0) {
>       int_field = 35;
>     } else {
>       int_field =222;
>     }
>   }
> 
> 
> 
> Test: tier1, application/ctw/modules

Why not do this (and the other similar #12978) by extending the logic where the merge happens in `PhaseIdealLoop::identical_backtoback_ifs()`?

I see a few reasons why that would be preferable:

- if the motivation is only to merge ifs, is it a good thing to transform the graph blindly during GVN when we don't know if the ifs are even candidates for merging?
- having that code far away from where the merge transformation happens makes understanding what's going on harder
- dropping a Cast node is risky, in general, because they sometimes carry a dependence that's required for correctness. So it should be done with care.

-------------

PR: https://git.openjdk.org/jdk/pull/13039


More information about the hotspot-compiler-dev mailing list