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

[Tobias Hartmann]

On Fri, 9 Dec 2022 09:02:27 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)
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   simplify with get_block_for_node

Much better, thanks for making these changes. Looks good.

src/hotspot/share/opto/cfgnode.hpp line 595:

> 593: public:
> 594:   NeverBranchNode( Node *ctrl ) : MultiBranchNode(1) {
> 595:     init_req(0,ctrl);

Suggestion:

  NeverBranchNode(Node* ctrl) : MultiBranchNode(1) {
    init_req(0, ctrl);

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

Marked as reviewed by thartmann (Reviewer).

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