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

[Vladimir Kozlov]

On Thu, 30 Jan 2025 10:03:49 GMT, Emanuel Peter <epeter 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
> #  assert(!VerifyNoNewIrreducibleLoops) failed: A new irreducible lo...

Few comments.

src/hotspot/share/opto/c2_globals.hpp line 636:

> 634:           "verify major loop optimizations")                                \
> 635:                                                                             \
> 636:   develop(bool, VerifyNoNewIrreducibleLoops, false,                         \

Do you plan to add it to our stress testing?

src/hotspot/share/opto/cfgnode.cpp line 445:

> 443: // - It is marked as "maybe irreducible entry". Any other loop status would guarantee
> 444: //   that it is never an irreducible loop entry.
> 445: // - It is not a LoopNode, those are guaranteed to be reducible loop entries.

Do you mean "**And** it is not a Loop node" (based on condition code below)?
Do we have an assert which checks that `LoopNode` is never marked `MaybeIrreducibleEntry`?

src/hotspot/share/opto/loopnode.cpp line 5635:

> 5633:       RegionNode* secondary_entry = m->as_Region();
> 5634: 
> 5635:       if (!secondary_entry->can_be_irreducible_entry()) {

Typo? Why negation?

src/hotspot/share/opto/loopnode.cpp line 5646:

> 5644:         l->_irreducible = 1; // = true
> 5645:         RegionNode* head = l->_head->as_Region();
> 5646:         if (!head->can_be_irreducible_entry()) {

Same issue here?

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

Changes requested by kvn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23363#pullrequestreview-2584545915
PR Review Comment: https://git.openjdk.org/jdk/pull/23363#discussion_r1935995783
PR Review Comment: https://git.openjdk.org/jdk/pull/23363#discussion_r1935989922
PR Review Comment: https://git.openjdk.org/jdk/pull/23363#discussion_r1935994030
PR Review Comment: https://git.openjdk.org/jdk/pull/23363#discussion_r1935994293