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

[Emanuel Peter]

On Mon, 3 Feb 2025 18:06:55 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>>> The issue is we should avoid creating new irreducible loops.
>> 
>> Absolutely. This is not a fix at all. But since we can catch that the state is wrong here, we should bail out instead of continuing on in production. That is at least a little improvement. The bug-fix would come in a second step, and may be much more complicated as it would have to reconsider what to do about `split_if`. Do we postpone until after loop-opts for example? Before considering such an involved fix, I would rather want to do this "defensive" patch first. Does that make sense? This also allows us to integrate [JDK-8348570](https://bugs.openjdk.org/browse/JDK-8348570), which is currently blocked by the failing assert (which is now disabled, but bailout instead, can be enabled with the flag).
>> 
>>> What if l is already marked as irreducible l->_irreducible = 1 by following code? I don't see check above for such case.
>> 
>> If `l->_irreducible = 1`, then the corresponding node should have been marked as `MaybeIrreducibleEntry`, and so should also `m` be marked with `MaybeIrreducibleEntry`. If that is the case, we are fine, these `Region` were created at parsing and we currently consider that fine.
>> 
>> But if one of the `Region` of the now irreducible loop was not marked accordingly already during parsing, then a new irreducible loop appeared during compilation - and that's not good.
>> 
>>> 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?
>> 
>> If we marked a `Region` with `MaybeIrreducibleEntry`, then we treat it differently in some optimizations. For example, when the region loses a control input, we have to check if the loop is now dead, with a global connectivity search. For `LoopNode` losing the control input means we already know that the loop is dead, since that was the only loop entry. But for irreducible loops, losing one entry means we do not know if there is still a secondary entry or not. For context see [JDK-8280126](https://bugs.openjdk.org/browse/JDK-8280126).
>> 
>> Does that answer your question? If not, we may have to talk about it offline ;)
>> 
>> PS:
>> The whole irreducible loop handling is still broken actually, see [JDK-8308675](https://bugs.openjdk.org/browse/JDK-8308675). We plan to eventually also disallow creation of irreducible loops at parsing. But that's a big project, and we were hoping for a student project. But we still also should not introduce new irreducible loop...
>
> First, I am fine with this "band-aid" change. I understand that it simple replaces assert with bailout which is fine.
> But I am trying to understand what it does.
> 
> There are few states when we come to this part of code:
>  - `l` is or not marked as irreducible
>  - `m` is or not marked with MaybeIrreducibleEntry (is it set only for not Loop?)
>  - `m` is or not Loop 
>  
>  So we have 8 combinations. I would like to hear reasons in which cases we should bailout and in which not.

@vnkozlov Does that help you, or do you have more questions?

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

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