RFR: 8371581: C2: PhaseCCP should reach fixpoint by revisiting deeply-Value-d nodes

[Emanuel Peter]

On Thu, 13 Nov 2025 10:49:14 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

> I started this as investigation into one rare/intermittent CTW failure that I get with [JDK-8360557](https://bugs.openjdk.org/browse/JDK-8360557). The bug seems to reproduce on a very specific JAR with a very specific random seed, so no easy regression test.
> 
> At this point I believe we found that PhaseCCP does not reach the fix point for a peculiar reason: `LoadN` that looks deeply into the graph is not revisited and thus misses the chance to update its type. There is an exception for loads in `verify_Value_for`, but it seems to only apply to constants, and does not apply to `LoadN` in question. Revisiting `LoadN` shows that updating the types downstream performs type widenings (= current types are too narrow), which AFAICS says that this unsound analysis can lead to miscompilation. See more debugging breadcrumbs in the bug.
> 
> It looks like we can reach the fixpoint by recording the nodes we need to revisit and doing another CCP round. This also makes CCP verification stricter: we effectively move 2 exceptional cases recorded in `verify_Value_for` into the analysis itself.
> 
> Testing shows there are no ill effects on correctness doing this. But I would appreciate someone more savvy in this code to sanity check all of this.
> 
> Additional testing:
>  - [x] Linux x86_64 server fastdebug, CTW reproducer no longer fails
>  - [x] Linux x86_64 server fastdebug, `all` tests pass
>  - [x] Linux x86_64 server fastdebug, Maven Central CTW passes (!)

@shipilev Nice work, looks like it was a big struggle with the reproducer. The randomness with the seed surely makes it more intermittent.

src/hotspot/share/opto/phaseX.cpp line 2850:

> 2848: // Add nodes here if particular *Node::Value is doing deep graph traversals
> 2849: // not handled by push_more_uses.
> 2850: bool PhaseCCP::needs_revisit(Node *n) const {

Suggestion:

bool PhaseCCP::needs_revisit(Node* n) const {

src/hotspot/share/opto/phaseX.cpp line 2856:

> 2854:   }
> 2855:   // CmpPNode performs deep traversals if it compares oopptr. CmpP is not notified for changes far away.
> 2856:   if (n->Opcode() == Op_CmpP) {

The verification restricts it to `n->Opcode() == Op_CmpP && type(n->in(1))->isa_oopptr() && type(n->in(2))->isa_oopptr()`. How big is the difference here? Might this have a performance impact?

src/hotspot/share/opto/phaseX.cpp line 2875:

> 2873:   // We should either make sure that these nodes are properly added back to the CCP worklist
> 2874:   // in PhaseCCP::push_child_nodes_to_worklist() to update their type in the same round,
> 2875:   // or that they are added in PhaseCCP::maybe_needs_revisit() so that analysis revisits

Suggestion:

  // or that they are added in PhaseCCP::needs_revisit() so that analysis revisits

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

PR Review: https://git.openjdk.org/jdk/pull/28288#pullrequestreview-3459633236
PR Review Comment: https://git.openjdk.org/jdk/pull/28288#discussion_r2523348552
PR Review Comment: https://git.openjdk.org/jdk/pull/28288#discussion_r2523341454
PR Review Comment: https://git.openjdk.org/jdk/pull/28288#discussion_r2523343785