RFR: 8296389: C2: PhaseCFG::convert_NeverBranch_to_Goto must handle both orders of successors

[Emanuel Peter]

On Tue, 6 Dec 2022 10:40:18 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> **Targetted for JDK21**
>> 
>> The code in `PhaseCFG::convert_NeverBranch_to_Goto` looks like it is ready to have `idx == 1`, but it is not.
>> 
>> We would read `succ` from `_succs[1]`.
>> https://github.com/openjdk/jdk/blob/8c472e481676ed0ef475c4989477d5714880c59e/src/hotspot/share/opto/block.cpp#L626
>> 
>> Then overwrite `_succs[0]` with `succ`, and shorten the array.
>> https://github.com/openjdk/jdk/blob/8c472e481676ed0ef475c4989477d5714880c59e/src/hotspot/share/opto/block.cpp#L635-L636
>> 
>> And finally attempt to read `dead` from `_succs[0]`, where the dead block used to be, but was just overwritten.
>> https://github.com/openjdk/jdk/blob/8c472e481676ed0ef475c4989477d5714880c59e/src/hotspot/share/opto/block.cpp#L645
>> 
>> **Solution**
>> Read `dead` before overwriting it. I also made it more robust by going via the projections, and not assuming that the projections and successors are ordered equally (though that is probably guaranteed by the matching traversal).
>> 
>> **Why did we never hit this bug before?**
>> Normal case: during matching, "succ" projection is added as output of NeverBranch before the "dead" projection leading to Halt. Thus, the outputs of NeverBranch are normally [[ "succ", "dead" ]], hence `idx == 0`.
>> Details: During DFS, usually we go from Halt to NeverBranch. Then via Region/Loop, take backedge, and find the "succ" edge. We already have its inputs (NeverBranch), thus we can now post-visit the live edge, and attach it to the NeverBranch first. Later, once we have processed the whole infinite loop, we post-visit out of NeverBranch to the "dead" projection edge, which we attach second.
>> 
>> Rare case: "dead" projection is first attached to NeverBranch, and "succ" projection is added second. We have [[ "dead", "succ" ]], hence `idx == 1`.
>> We have a peeled infinite loop. The NeverBranch of the peeled iteration is first visited via the "dead" projection from HaltNode. Since the peeled iteration has no backedge, we do not visit the "succ" projection yet, but instead attach "dead" projection to HaltNode already once we are done visiting everything above. Later, we come from the peeled loop's NeverBranch exit, to the "succ" projection of the peeled iteration's NeverBranch, and attach the "succ" projection.
>> 
>> ![image](https://user-images.githubusercontent.com/32593061/205299027-0e8e1d46-a49c-48c6-81b4-dfe83d8236ec.png)
>
> test/hotspot/jtreg/compiler/loopopts/TestPhaseCFGNeverBranchToGotoMain.java line 28:
> 
>> 26:  * @bug 8296389
>> 27:  * @summary Peeling of Irreducible loop can lead to NeverBranch being visited from either side
>> 28:  * @run main/othervm -Xcomp -Xbatch -XX:-TieredCompilation -XX:PerMethodTrapLimit=0
> 
> Suggestion:
> 
>  * @run main/othervm -Xcomp -XX:-TieredCompilation -XX:PerMethodTrapLimit=0
> 
> 
> `-Xcomp` implies `-Xbatch`

👍

> test/hotspot/jtreg/compiler/loopopts/TestPhaseCFGNeverBranchToGotoMain.java line 38:
> 
>> 36:  * @compile TestPhaseCFGNeverBranchToGoto.jasm
>> 37:  * @summary Peeling of Irreducible loop can lead to NeverBranch being visited from either side
>> 38:  * @run main/othervm -Xcomp -Xbatch -XX:-TieredCompilation -XX:PerMethodTrapLimit=0
> 
> Suggestion:
> 
>  * @run main/othervm -Xcomp -XX:-TieredCompilation -XX:PerMethodTrapLimit=0

👍

> test/hotspot/jtreg/compiler/loopopts/TestPhaseCFGNeverBranchToGotoMain.java line 48:
> 
>> 46:         test(false, false);
>> 47:     }
>> 48:     public static void test(boolean flag1, boolean flag2) {
> 
> Suggestion:
> 
>     }
> 
>     public static void test(boolean flag1, boolean flag2) {

👍

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

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