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

[Marc Chevalier]

On Tue, 30 Sep 2025 06:22:52 GMT, Tobias Hartmann <thartmann 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...
>
> src/hotspot/share/opto/cfgnode.cpp line 1482:
> 
>> 1480:   }
>> 1481:   {
>> 1482:     Node* uin = unique_constant_input_recursive(phase);
> 
> I think the original format was fine but I have no strong opinion.

I wouldn't fight too hard for it, but when trying things, it was necessary (since I wanted different types for `uin`), but I also think it helps to see that it is not useful too long, and I can mess with it without having to look at what comes after. In this case, indeed, it feels a bit coming out of nowhere at the end...

In C++17, I'd do:
```c++
 if (Node* uin = unique_constant_input_recursive(phase); uin != nullptr) {
  return uin;
}

one could also do
```c++
 if (Node* uin = unique_constant_input_recursive(phase)) {
  return uin;
}

if we are not afraid of implicit conditions (but we are, and I'm fine with it!).

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1640#discussion_r2390390461