[lworld] RFR: 8367242: [lworld] C2 compilation asserts with "dead loop detected" [v3]

[duke]

On Wed, 1 Oct 2025 09:35:35 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> ## Act I: dead loops
>> 
>> [JDK-8336003: [lworld] TestLWorld::test151 triggers "Should have been buffered" assert](https://bugs.openjdk.org/browse/JDK-8336003), allows to replace a Phi node by a single input recursively through phis. This change was only added to Valhalla. In mainline, a Phi can be simplified to its single direct input, not through other phis. The valhalla version will work on loops, while the mainline version will reduce all the phis into a single input only if the blob of Phis is acyclic.
>> 
>> When a loop is dead, it is possible that the said single input is also an output of the phi. After applying the identity, the phi's unique input becomes its own output (or input). For most nodes, that is not allowed. This is probably relatively harmless as the whole code around is dead and will be removed, but it makes some verification fail. It is also possible that some other idealization are not protected against data loops without phis on the way, and would not terminate. It is interesting to notice that applying [JDK-8336003](https://bugs.openjdk.org/browse/JDK-8336003) to mainline is enough to reproduce the bug there too.
>> 
>> We could detect when it's about to happen, and handle the situation differently if we are about to create a very small loop that would be caught by the dead loop detection. It would be possible to make big dead data loop. How annoying that is? Immediately, there is the non-termination problem mentioned above. But also, maybe some nodes can be optimized away by IGVN and end up with a small loop and then the assert would strike again. Is the dead loop check too weak then? It depends what we think is the purpose of this check. During IGVN, we cannot clean up eagerly every dead loop since it would be too expensive to traverse everything. Avoiding dead data loop would also need a lot of traversal. My understanding is that it's rather a sanity check, to make sure that one doesn't mess up the graph surgery and create dead loops accidentally, when something else was meant, and detecting as soon as possible is helpful. So I'm not sure it's worth strengthening the check.
>> 
>> A way to avoid the creation of dead data loop is to simply limit the simplification allowed by [JDK-8336003](https://bugs.openjdk.org/browse/JDK-8336003) to constant nodes: since they don't have input, they can't make a cycle! And it seems enough for the bug initially reported.
>> 
>> Yet, that makes `test151` in `compiler/valhalla/inlinetypes/TestLWorld.java...
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   More comments

@marc-chevalier 
Your change (at version 161cae4d677c651d7e2b6739237433ac4b33bbaf) is now ready to be sponsored by a Committer.

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

PR Comment: https://git.openjdk.org/valhalla/pull/1640#issuecomment-3359457258