From 99be0e73ce9779e85c9ec6598e0a7ce964d62e82 Mon Sep 17 00:00:00 2001 From: Aleksey Shipilev Date: Mon, 24 Nov 2025 07:47:13 +0000 Subject: [PATCH] 8371581: C2: PhaseCCP should reach fixpoint by revisiting deeply-Value-d nodes Reviewed-by: epeter, vlivanov, qamai --- src/hotspot/share/opto/phaseX.cpp | 84 +++++++++++++++++++++++-------- src/hotspot/share/opto/phaseX.hpp | 4 +- 2 files changed, 66 insertions(+), 22 deletions(-) diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index 1fe911aa7ac..4a0933b89f2 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -1132,7 +1132,7 @@ void PhaseIterGVN::verify_empty_worklist(Node* node) { // (1) Integer "widen" changes, but the range is the same. // (2) LoadNode performs deep traversals. Load is not notified for changes far away. // (3) CmpPNode performs deep traversals if it compares oopptr. CmpP is not notified for changes far away. -bool PhaseIterGVN::verify_Value_for(Node* n) { +bool PhaseIterGVN::verify_Value_for(Node* n, bool strict) { // If we assert inside type(n), because the type is still a null, then maybe // the node never went through gvn.transform, which would be a bug. const Type* told = type(n); @@ -1152,7 +1152,7 @@ bool PhaseIterGVN::verify_Value_for(Node* n) { } // Exception (2) // LoadNode performs deep traversals. Load is not notified for changes far away. - if (n->is_Load() && !told->singleton()) { + if (!strict && n->is_Load() && !told->singleton()) { // MemNode::can_see_stored_value looks up through many memory nodes, // which means we would need to notify modifications from far up in // the inputs all the way down to the LoadNode. We don't do that. @@ -1160,7 +1160,7 @@ bool PhaseIterGVN::verify_Value_for(Node* n) { } // Exception (3) // CmpPNode performs deep traversals if it compares oopptr. CmpP is not notified for changes far away. - if (n->Opcode() == Op_CmpP && type(n->in(1))->isa_oopptr() && type(n->in(2))->isa_oopptr()) { + if (!strict && n->Opcode() == Op_CmpP && type(n->in(1))->isa_oopptr() && type(n->in(2))->isa_oopptr()) { // SubNode::Value // CmpPNode::sub // MemNode::detect_ptr_independence @@ -2799,6 +2799,7 @@ void PhaseCCP::analyze() { // Compile is over. The local arena gets de-allocated at the end of its scope. ResourceArea local_arena(mtCompiler); Unique_Node_List worklist(&local_arena); + Unique_Node_List worklist_revisit(&local_arena); DEBUG_ONLY(Unique_Node_List worklist_verify(&local_arena);) // Push root onto worklist @@ -2807,45 +2808,86 @@ void PhaseCCP::analyze() { assert(_root_and_safepoints.size() == 0, "must be empty (unused)"); _root_and_safepoints.push(C->root()); - // Pull from worklist; compute new value; push changes out. - // This loop is the meat of CCP. + // This is the meat of CCP: pull from worklist; compute new value; push changes out. + + // Do the first round. Since all initial types are TOP, this will visit all alive nodes. while (worklist.size() != 0) { Node* n = fetch_next_node(worklist); DEBUG_ONLY(worklist_verify.push(n);) + if (needs_revisit(n)) { + worklist_revisit.push(n); + } if (n->is_SafePoint()) { // Make sure safepoints are processed by PhaseCCP::transform even if they are // not reachable from the bottom. Otherwise, infinite loops would be removed. _root_and_safepoints.push(n); } - const Type* new_type = n->Value(this); - if (new_type != type(n)) { - DEBUG_ONLY(verify_type(n, new_type, type(n));) - dump_type_and_node(n, new_type); - set_type(n, new_type); - push_child_nodes_to_worklist(worklist, n); - } - if (KillPathsReachableByDeadTypeNode && n->is_Type() && new_type == Type::TOP) { - // Keep track of Type nodes to kill CFG paths that use Type - // nodes that become dead. - _maybe_top_type_nodes.push(n); - } + analyze_step(worklist, n); } + + // More rounds to catch updates far in the graph. + // Revisit nodes that might be able to refine their types at the end of the round. + // If so, process these nodes. If there is remaining work, start another round. + do { + while (worklist.size() != 0) { + Node* n = fetch_next_node(worklist); + analyze_step(worklist, n); + } + for (uint t = 0; t < worklist_revisit.size(); t++) { + Node* n = worklist_revisit.at(t); + analyze_step(worklist, n); + } + } while (worklist.size() != 0); + DEBUG_ONLY(verify_analyze(worklist_verify);) } +void PhaseCCP::analyze_step(Unique_Node_List& worklist, Node* n) { + const Type* new_type = n->Value(this); + if (new_type != type(n)) { + DEBUG_ONLY(verify_type(n, new_type, type(n));) + dump_type_and_node(n, new_type); + set_type(n, new_type); + push_child_nodes_to_worklist(worklist, n); + } + if (KillPathsReachableByDeadTypeNode && n->is_Type() && new_type == Type::TOP) { + // Keep track of Type nodes to kill CFG paths that use Type + // nodes that become dead. + _maybe_top_type_nodes.push(n); + } +} + +// Some nodes can refine their types due to type change somewhere deep +// in the graph. We will need to revisit them before claiming convergence. +// Add nodes here if particular *Node::Value is doing deep graph traversals +// not handled by PhaseCCP::push_more_uses(). +bool PhaseCCP::needs_revisit(Node* n) const { + // LoadNode performs deep traversals. Load is not notified for changes far away. + if (n->is_Load()) { + return true; + } + // CmpPNode performs deep traversals if it compares oopptr. CmpP is not notified for changes far away. + if (n->Opcode() == Op_CmpP && type(n->in(1))->isa_oopptr() && type(n->in(2))->isa_oopptr()) { + return true; + } + return false; +} + #ifdef ASSERT // For every node n on verify list, check if type(n) == n->Value() -// We have a list of exceptions, see comments in verify_Value_for. +// Note for CCP the non-convergence can lead to unsound analysis and mis-compilation. +// Therefore, we are verifying Value convergence strictly. void PhaseCCP::verify_analyze(Unique_Node_List& worklist_verify) { bool failure = false; while (worklist_verify.size()) { Node* n = worklist_verify.pop(); - failure |= verify_Value_for(n); + failure |= verify_Value_for(n, /* strict = */ true); } // If we get this assert, check why the reported nodes were not processed again in CCP. // We should either make sure that these nodes are properly added back to the CCP worklist - // in PhaseCCP::push_child_nodes_to_worklist() to update their type or add an exception - // in the verification code above if that is not possible for some reason (like Load nodes). + // in PhaseCCP::push_child_nodes_to_worklist() to update their type in the same round, + // or that they are added in PhaseCCP::needs_revisit() so that analysis revisits + // them at the end of the round. assert(!failure, "PhaseCCP not at fixpoint: analysis result may be unsound."); } #endif diff --git a/src/hotspot/share/opto/phaseX.hpp b/src/hotspot/share/opto/phaseX.hpp index 083e77bf6d9..473231e6af5 100644 --- a/src/hotspot/share/opto/phaseX.hpp +++ b/src/hotspot/share/opto/phaseX.hpp @@ -490,7 +490,7 @@ public: void optimize(); #ifdef ASSERT void verify_optimize(); - bool verify_Value_for(Node* n); + bool verify_Value_for(Node* n, bool strict = false); bool verify_Ideal_for(Node* n, bool can_reshape); bool verify_Identity_for(Node* n); void verify_empty_worklist(Node* n); @@ -659,6 +659,8 @@ class PhaseCCP : public PhaseIterGVN { // Worklist algorithm identifies constants void analyze(); + void analyze_step(Unique_Node_List& worklist, Node* n); + bool needs_revisit(Node* n) const; #ifdef ASSERT void verify_type(Node* n, const Type* tnew, const Type* told); // For every node n on verify list, check if type(n) == n->Value()