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

Emanuel Peter epeter at openjdk.org
Fri Dec 9 07:07:14 UTC 2022


> **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)

Emanuel Peter 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 four additional commits since the last revision:

 - Refactoring a bit after review suggestions
 - Merge branch 'master' into JDK-8296389
 - replace tabs with spaces
 - 8296389: C2: PhaseCFG::convert_NeverBranch_to_Goto must handle both orders of successors

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/11481/files
  - new: https://git.openjdk.org/jdk/pull/11481/files/c826a8ff..f1d25d0e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11481&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11481&range=00-01

  Stats: 41228 lines in 988 files changed: 25126 ins; 9939 del; 6163 mod
  Patch: https://git.openjdk.org/jdk/pull/11481.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11481/head:pull/11481

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


More information about the hotspot-compiler-dev mailing list