RFR: 8319372: C2 compilation fails with "Bad immediate dominator info"

[Christian Hagedorn]

On Tue, 28 Nov 2023 10:55:13 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> I included the test case from 8318690 because the cause of the failure
> is similar, the fix I propose applies to both. If this fix goes in
> then 8318690 can be closed as duplicate of this one.
> 
> In both cases, a `CastII`'s type is narrowed because it is control
> dependent on a condition that tests the input of the `CastII` (code in
> `CastIINode::Value()`). In both cases, that `CastII` is input to a
> range check `CastII`. Predication leaves that second `CastII` in the
> loop body but the range check that guards it is replaced by
> predicates. The difference in the 2 test cases is where the first
> `CastII` comes from: `PhiNode::Ideal()` in one case,
> `PhaseIdealLoop::try_sink_out_of_loop()` in the other. The narrowed
> type of the first `CastII` conflicts with the type of the range check
> `CastII` and that causes that one to become top. The path where that
> `CastII` is is unreachable but c2 can't prove it. The inconsistency
> causes the asserts.
> 
> To fix this we would need to prove that the path where the `CastII`
> nodes are is dead and for that we would need to prove that the
> condition that guards that path and at least one of the predicates
> can't be true at the same time. While it may be feasible, that seems
> like a lot of extra complexity.
> 
> Instead I propose removing the logic in `CastIINode::Value()`
> entirely. I went back to the change that introduced it and, as I
> understand, it was added as an attempt to detect and remove useless
> `CastII` nodes. There was no correctness issue involved AFAICT.

I found myself before at that point to just get rid of this code to fix [JDK-8272562](https://bugs.openjdk.org/browse/JDK-8272562) back there (also see https://github.com/openjdk/jdk/pull/5199#pullrequestreview-735143057).  It's a trade-off between better type information and avoiding these kind of issues. However, we could fix some cases by restricting `try_sink_out_of_loop()`. However, there was still this [test](https://bugs.openjdk.org/secure/attachment/101741/Reduced2.java)

$ java -Xcomp -XX:CompileOnly=Reduced*::* -XX:-TieredCompilation Reduced2.java

which fails with mainline today but would work with your fix. You might want to add this test case as well. 

Maybe we need to step through all the reported test cases which we closed as duplicates of [JDK-8275202](https://bugs.openjdk.org/browse/JDK-8275202) at some point and see if this patch can fix them as well. I have the feeling that it can.

Additionally, we might want to investigate separately at some point if this patch allows us to relax some of the constraints of `try_sink_out_of_loop()` which we added fix these kind of issues with `CastII` nodes. I'm not sure though how much benefit it will bring.

Anyway, the fix looks good to me!

test/hotspot/jtreg/compiler/c2/TestTopCastIIOnUndetectedDeadPath.java line 31:

> 29:  *                   -XX:+UnlockDiagnosticVMOptions -XX:StressSeed=426264791 -XX:+StressIGVN TestTopCastIIOnUndetectedDeadPath
> 30:  * @run main/othervm -Xcomp -XX:CompileOnly=TestTopCastIIOnUndetectedDeadPath::test -XX:CompileCommand=quiet -XX:-TieredCompilation
> 31:  *                   -XX:+UnlockDiagnosticVMOptions -XX:+StressIGVN TestTopCastIIOnUndetectedDeadPath

Since `StressIGVN` is a C2 flag, you should also add a `@requires vm.compiler2.enabled`. Same for the other test.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16844#pullrequestreview-1754657728
PR Review Comment: https://git.openjdk.org/jdk/pull/16844#discussion_r1408895684