RFR: 8291775: C2: assert(r != __null && r->is_Region()) failed: this phi must have a region

[Vladimir Kozlov]

On Mon, 15 Aug 2022 12:42:04 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> [JDK-8271341](https://bugs.openjdk.org/browse/JDK-8271341) fixed a case for string opts where a diamond `Region/If` in the middle of a chain of calls that are optimized out is not folded away correctly in IGVN, leading to a broken graph. The problem in the test case of JDK-8271341 was the following: 
> 
> One of the inputs of the `Cmp` nodes of the `If` is replaced by top (the data projection of the removed call into the `Cmp` node). During IGVN, top propagated too quickly to the `If` before the `Region/If` diamond could be folded away. As a result, the control flow below the `If` became dead. The fix was to eagerly fold the `Region/If` diamond in string opts by replacing the `Region` by the control input of the `If`  (see https://github.com/openjdk/jdk/pull/4944):
> 
> ![Screenshot from 2022-08-15 11-24-31](https://user-images.githubusercontent.com/17833009/184611029-72852d8a-aaef-40bd-b792-a6e6fa1b33aa.png)
> 
> Replaced `396 Region` by control input of `If` (`330 IfTrue`):
> 
> ![Screenshot from 2022-08-15 11-24-47](https://user-images.githubusercontent.com/17833009/184611053-678afd48-03c5-4c79-b14a-28ab7d5c1b5c.png)
> 
> The reason why we can rewire the phis to a non-region during string opts is because all phis are dead and will be removed. In the testcase of JDK-8271341, this is done in `PhaseRemoveUseless` because the data `424 Phi` has no users after string opts anymore. 
> 
> However, in the test case of this bug (actually the same test case but run with `-XX:+AlwaysIncrementalInline`), we have an additional memory phi which is actually dead but not removed by `PhaseRemoveUseless` because it still has other users at that point. The memory phi would eventually be removed during IGVN but the problem is that it is unexpected to process a phi with a non-region control input and we fail with the assertion in `PhiNode::Ideal()`.
> 
> The fix I propose is to not replace the `Region` node with the control input of the `If` as in JDK-8271341 but instead setting the boolean input of the `If` node to a constant (doesn't matter if 0 or 1 since the phis are dead anyways). If we then process the `If` or `IfTrue/IfFalse` before the `Region` in IGVN, we simply fold the node. This allows IGVN to safely remove a diamond `Region/If` without interference of top from string opts. The memory phi is eventually removed during IGVN.
> 
> Thanks,
> Christian

Good.

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

Marked as reviewed by kvn (Reviewer).

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