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

[Emanuel Peter]

On Fri, 2 Dec 2022 12:48:31 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> 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).
> 
> **Refactoring: added class id for NeverBranch**
> I also added the class id for NeverBranch, and replaced all `Op_NeverBranch` checks with `is_NeverBranch()`.
> 
> **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)

This pull request has now been integrated.

Changeset: fabda246
Author:    Emanuel Peter <epeter at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/fabda246960cfdfff13c5a87de53d97169248172
Stats:     188 lines in 11 files changed: 167 ins; 2 del; 19 mod

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

Reviewed-by: thartmann, chagedorn

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

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