RFR: 8335708: C2: Compile::verify_graph_edges must start at root and safepoints, just like CCP traversal [v2]

[Christian Hagedorn]

On Tue, 18 Mar 2025 13:57:33 GMT, Marc Chevalier <duke at openjdk.org> wrote:

>> In CCP, we transform the nodes going up (toward inputs) starting from root and safepoints because infinite loops can be reachable from the root, but not co-reachable from the root, that is one can follow def-use from root to the loop, but not the use-def from root to loop. For more details, see:
>> https://github.com/openjdk/jdk/blob/4cf63160ad575d49dbe70f128cd36aba22b8f2ff/src/hotspot/share/opto/phaseX.cpp#L2063-L2070
>> 
>> Since we are specifically marking nodes as useful if they are above a safepoint, the check that no dead nodes must be there anymore must also consider nodes above a safepoint as alive: the same criterion must apply. We should nevertheless not start from a safepoint killed by CCP.
>> 
>> About the test, I use this trick found in `TestInfiniteLoopCCP` because I indeed need a really infinite loop, but I want a terminating test. The crash is not deterministic, as it needs StressIGVN, so I did a bit of stats. Using a little helper script, on 100 runs, 69 runs fail as in the JBS ticket and 31 are successful (so 0 fail in another way). After the fix, I find 100 successes.
>> 
>> And thanks to @eme64  who extracted such a concise reproducer.
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   various fixes

Some minor comments. Otherwise, looks good to me, too!

src/hotspot/share/opto/compile.cpp line 4208:

> 4206:   if (root_and_safepoints != nullptr) {
> 4207:     assert(root_and_safepoints->member(_root), "root is not in root_and_safepoints");
> 4208:     for (unsigned int i = 0, limit = root_and_safepoints->size(); i < limit; i++) {

You should use `uint` instead:
Suggestion:

    for (uint i = 0, limit = root_and_safepoints->size(); i < limit; i++) {

src/hotspot/share/opto/compile.cpp line 4210:

> 4208:     for (unsigned int i = 0, limit = root_and_safepoints->size(); i < limit; i++) {
> 4209:       Node* root_or_safepoint = root_and_safepoints->at(i);
> 4210:       // If the node is a safepoint, let's check it still has a control input

Suggestion:

      // If the node is a safepoint, let's check if it still has a control input.

src/hotspot/share/opto/compile.cpp line 4211:

> 4209:       Node* root_or_safepoint = root_and_safepoints->at(i);
> 4210:       // If the node is a safepoint, let's check it still has a control input
> 4211:       // Lack of control input signified that this node was killed by CCP or

Suggestion:

      // Lack of control input signifies that this node was killed by CCP or

src/hotspot/share/opto/compile.hpp line 1242:

> 1240:   //
> 1241:   // To call this function, there are 2 ways to go:
> 1242:   // - give root_and_safepoints to start traversal everywhere needed (like after CCP),

Suggestion:

  // - give root_and_safepoints to start traversal everywhere needed (like after CCP)

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23977#pullrequestreview-2702571571
PR Review Comment: https://git.openjdk.org/jdk/pull/23977#discussion_r2005557198
PR Review Comment: https://git.openjdk.org/jdk/pull/23977#discussion_r2005558259
PR Review Comment: https://git.openjdk.org/jdk/pull/23977#discussion_r2005558468
PR Review Comment: https://git.openjdk.org/jdk/pull/23977#discussion_r2005560678