RFR: 8278228: C2: Improve identical back-to-back if elimination [v3]

[Christian Hagedorn]

On Wed, 5 Jan 2022 17:06:57 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> C2 has had the ability to optimize:
>> 
>> (1)
>> 
>> if (some_condition) {
>>   // body 1
>> } else {
>>   // body 2
>> }
>> if (some_condition) {
>>   // body 3
>> } else {
>>   // body 4
>> }
>> 
>> into:
>> 
>> (4)
>> 
>> if (some_condition) {
>>   // body 1
>>   // body 3
>> } else {
>>   // body 2
>>   // body 4
>> }
>> 
>> for a while.
>> 
>> This is achieved by the intermediate step:
>> 
>> (2)
>> 
>> if (some_condition) {
>>   // body 1
>>   some_condition2 = true;
>> } else {
>>   // body 2
>>   some_condition2 = false;
>> }
>> if (some_condition2) {
>>   // body 3
>> } else {
>>   // body 4
>> }
>> 
>> which then allows the use of the exiting split if optimization. As a
>> result, the graph is transformed to:
>> 
>> (3)
>> 
>> if (some_condition) {
>>   // body 1
>>   some_condition2 = true;
>>   if (some_condition2) {
>>     body3: // a Region here
>>     // body3
>>   } else {
>>     goto body4;
>>   }
>> } else {
>>   // body 2
>>   some_condition2 = false;
>>   if (some_condition2) {
>>     goto body3;
>>   } else {
>>     body4:  // another Region here
>>     // body4;
>>   }
>> }
>> 
>> and finally to (4) above.
>> 
>> Recently, 8275610 has shown that this can break if some_condition is a
>> null check. If, say, body 3 has a control dependent CastPP, then when
>> body 1 and body 3 are merged, the CastPP of body 3 doesn't become
>> control dependent on the dominating if (because in step (2), the
>> CastPP hides behind a Region). As a result, the CastPP loses its
>> dependency on the null check.
>> 
>> After discussing this with Christian, it seemed this was caused by the
>> way this transformation relies on split if: having custom code that
>> wouldn't create Regions at body3 and body4 that are then optimized out
>> would solve the problem. Anyway, after looking at the split if code,
>> trying to figure out how to tease it apart in smaller steps and
>> reusing some of them to build a new transformation, it seemed too
>> complicated. So instead, I propose reusing split if in a slightly
>> different way:
>> 
>> skip step (2) but perform split if anyway to obtain:
>> 
>> if (some_condition) {
>>   // body 1
>>   if (some_condition) {
>>     body3: // Region1
>>     // CastPP is here, control dependent on Region1
>>     // body3
>>   } else {
>>     goto body4;
>>   }
>> } else {
>>   // body 2
>>  if (some_condition) {
>>     goto body3;
>>   } else {
>>     body4:  // Region2
>>     // body4;
>>   }
>> }
>> 
>> - A CastPP node would still be behind a Region. So next step, is to push
>> control dependent nodes through Region1 and Region2:
>> 
>> if (some_condition) {
>>   // body 1
>>   if (some_condition) {
>>     // A CastPP here
>>     body3: // Region1
>>     // body3
>>   } else {
>>     goto body4;
>>   }
>> } else {
>>   // body 2
>>  if (some_condition) {
>>     // A CastPP here
>>     goto body3;
>>   } else {
>>     body4:  // Region2
>>     // body4;
>>   }
>> }
>> 
>> - And then call dominated_by() to optimize the dominated:
>> 
>> if (some_condition) {
>> 
>> in both branches of the dominating if (some_condition) {. That also
>> causes the CastPP to become dependent on the dominating if.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
> 
>  - Christian's review
>  -  Merge branch 'master' into JDK-8278228
>  - only for nodes that depends_only_on_test()
>  - more
>  - more
>  - more
>  - more
>  - exps

Thanks for doing the updates! That's even better to move it to a separate method.

Two more things I've noticed:
https://github.com/openjdk/jdk/blob/bc12381105ef5ba14f99304a220817e97c9a99b5/src/hotspot/share/opto/split_if.cpp#L445
This could also be changed to `RegionNode*` with an assertion moved from
https://github.com/openjdk/jdk/blob/bc12381105ef5ba14f99304a220817e97c9a99b5/src/hotspot/share/opto/split_if.cpp#L37

And `new_iff` could also be changed to `RegionNode*`:
https://github.com/openjdk/jdk/blob/bc12381105ef5ba14f99304a220817e97c9a99b5/src/hotspot/share/opto/split_if.cpp#L494

Will submit some testing and get back with the results.

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

PR: https://git.openjdk.java.net/jdk/pull/6882