RFR: 8348572: C2 compilation asserts due to unexpected irreducible loop [v2]

[Vladimir Kozlov]

On Fri, 31 Jan 2025 05:54:03 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> 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?
>
> We cannot currently do that:
> 
>> With the new debug flag -XX:+VerifyNoNewIrreducibleLoops, we still hit the assert, as expected:
> 
> And we can do this later:
> 
>> 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.
> 
> Does that sound ok to you?

Sounds good. My question was about your future plan when "everything" is fixed.

>> 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?
>
> If a secondary entry can not be a secondary (irreducible) entry, then we have a problem, so we do a bailout.
> 
> Does that make sense?

Not completely.

The issue is we should avoid creating new irreducible loops.
What if `l` is already marked as irreducible `l->_irreducible = 1` by following code?  I don't see check above for such case.
And again come here but secondary_entry can be irreducible (it is Region for example or already marked) and we skip bailout.  Why it is okay?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23363#discussion_r1937799401
PR Review Comment: https://git.openjdk.org/jdk/pull/23363#discussion_r1937862215