On Fri, 26 Sep 2025 14:02:35 GMT, Marc Chevalier <[email protected]> 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` > fail in another way:... Nice analysis Marc! Let's see if the current solution is sufficient :) I just have a few style comments. 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. src/hotspot/share/opto/inlinetypenode.cpp line 171: > 169: > 170: // Merges 'this' with 'top' by updating the input PhiNodes added by > 'clone_with_phis' > 171: InlineTypeNode* InlineTypeNode::merge_with_top(PhaseGVN* gvn, int pnum, > bool transform) { I don't like the code duplication with `InlineTypeNode::merge_with` here. Would it be feasible to merge the two and have `InlineTypeNode::merge_with` handle `top` for `other`? Maybe the `PhiNode` inputs and `val2`can be initialized to `top` by default (maybe already in `clone_with_phis`) such that we only need to guard the `set_req` calls by `!other->is_top()`? src/hotspot/share/opto/phaseX.cpp line 783: > 781: } > 782: } > 783: if (!no_dead_loop) n->dump_bfs(100,nullptr,""); This was fixed upstream by https://github.com/openjdk/jdk/commit/2826d1702534783023802ac5c8d8ea575558f09f, no need to cherry pick. ------------- Marked as reviewed by thartmann (Committer). PR Review: https://git.openjdk.org/valhalla/pull/1640#pullrequestreview-3282769618 PR Review Comment: https://git.openjdk.org/valhalla/pull/1640#discussion_r2389990154 PR Review Comment: https://git.openjdk.org/valhalla/pull/1640#discussion_r2389984465 PR Review Comment: https://git.openjdk.org/valhalla/pull/1640#discussion_r2389989039
