RFR: 8348572: C2 compilation asserts due to unexpected irreducible loop

[Emanuel Peter]

On Thu, 30 Jan 2025 11:37:16 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> A quick summary:
>> - In [JDK-8280126](https://bugs.openjdk.org/browse/JDK-8280126), we decided that we are only going to allow irreducible loops that were detected at parsing, and we can thus restrict optimizations to reducible loops which would be difficult to do correct with irreducible loops. That's why we added that assert that checks that no new irreducible loop shows up during compilation.
>> - Problem: we use `split_if` for `IfNode::Ideal_common` to split through a Region that is loop-head, and the splitting of the Region introduces a second loop entry -> irreducible loop.
>> 
>> Before `split_if`:
>> ![image](https://github.com/user-attachments/assets/01bc78fa-7fed-4a8f-b6f4-078dac9b5dc4)
>> 
>> After `split_if`:
>> ![image](https://github.com/user-attachments/assets/1e3bd08e-b76d-4e7f-813e-27a5a22cb2bd)
>> 
>> 
>> - We have the `split_if` for `IfNode::Ideal_common` to do split-if on straight-line code. But we currently execute this before loop-opts, and so we don't know if the region we split through is actually a loop head. We guard against LoopNode, but a Region only becomes a LoopNode in loop-opts.
>> - We also have split-if in loop-opts, which is more careful about splitting through loop-heads.
>> - Just removing the straight-line split-if probably leads to a regression, as the loop-opts version only executes if there are loops for example.
>> - We could consider delaying the straight-line split-if until after loop-opts. But I don't know if that could lead to regressions in any way.
>> 
>> I discussed this temporary solution with @TobiHartmann :
>> - We would like [JDK-8348570](https://bugs.openjdk.org/browse/JDK-8348570) to be unblocked for @shipilev .
>> - Convert the assert into a bailout-check, so we are sure we behave correctly in product. Compiling with irreducible loops behaves correctly in almost all cases, but there could be exceptions.
>> - For now, have the assert behind a Verify flag, so that [JDK-8348570](https://bugs.openjdk.org/browse/JDK-8348570) is unblocked. Later, we can remove the Verify flag and alway enable the assert again.
>> - This fix also looks easier to backport.
>> 
>> -----------------------
>> 
>> The attached regression test now does **NOT** fail by default, but rather silently bails out of compilation.
>> 
>> With the new debug flag `-XX:+VerifyNoNewIrreducibleLoops`, we still hit the assert, as expected:
>> 
>> #  Internal Error (/oracle-work/jdk-fork0/open/src/hotspot/share/opto/loopnode.cpp:5636), pid=3698055, tid=3698072
>> #  asser...
>
> Is there any way to restrict the optimization to not-before-loopopts instead of after-loopopts? I assume after loop detection we are fine?

@merykitty I'm not sure if this is still the case:
With infinite loops, we would only detect the infinite loop, but not the inner loops. We would create a NeverBranch, so that in the next loop-opts iteration the infinite loop does not look like an infinite loop any more, and we can find the inner loops now. If that is still the case, then we would actually never be quite sure if a Region could not become a LoopNode later on. And your suggestion would not work.

@rwestrel do you have any idea / opinion?

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

PR Comment: https://git.openjdk.org/jdk/pull/23363#issuecomment-2624330400