RFR: 8359602: VerifyIterativeGVN fails with assert(!VerifyHashTableKeys || _hash_lock == 0) failed: remove node from hash table before modifying it

[Marc Chevalier]

On Fri, 27 Jun 2025 10:59:57 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

> This PR prevents some missed ideal optimizations in IGVN by notifying users of type refinements made during CCP, addressing a missed optimization that caused a verification failure with `-XX:VerifyIterativeGVN=1110`.
> 
> ### Context
> During the compilation of the input program (obtained from the fuzzer, then simplified and added as a test) by C2, we end up with node `591 ModI` that takes `138 Phi` as its divisor input. An existing `Ideal` optimization is to get rid of the control input of a `ModINode` when we can prove that the divisor is never `0`.
> 
> In this specific case, the type of the `PhiNode` gets refined during CCP, but the refinement fails to propagate to its users for the IGVN phase and the ideal optimization for the `ModINode` never happens. This results in a missed optimization and hits an assert in the verification phase of IGVN (when using `-XX:VerifyIterativeGVN=1110`).
> 
> ![IGV screenshot](https://github.com/user-attachments/assets/5dee1ae6-9146-4115-922d-df33b7ccbd37)
> 
> ### Detailed Analysis
> 
> In `PhaseCCP::analyze`, we call `Value` for the `PhiNode`, which  
> results in a type refinement: the range gets restricted to `int:-13957..-1191`.
> 
> ```c++
>   // Pull from worklist; compute new value; push changes out.
>   // This loop is the meat of CCP.
>   while (worklist.size() != 0) {
>     Node* n = fetch_next_node(worklist);
>     DEBUG_ONLY(worklist_verify.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);
>     }
>   }
>   DEBUG_ONLY(verify_analyze(worklist_verify);)
> 
> 
> At the end of `PhaseCCP::analyze`, we obtain the following types in the side table:
> - `int` for node `591` (`ModINode`)
> - `int:-13957..-1191` for node `138` (`PhiNode`)
> 
> If we call `find_node(138)->bottom_type()`, we get:
> - `int` for both nodes
> 
> There is no progress on the type of `ModINode` during CCP, because `ModINode::Value`  
> is not able to...

Wow, nice explanation, I like it! And solution seems good. We do enqueue users for IGVN when value is refined in IGVN, it's not shocking it's also needed to enqueue users for IGVN when the type improvement doesn't come from IGVN: we need to idealize the users because the abstract value improved, not because it was improved during IGVN in particular.

A little question tho.

test/hotspot/jtreg/compiler/c2/TestPropagateTypeRefinementToIGVN.java line 30:

> 28:  *          possible to prove that the divisor can never be 0.
> 29:  *          VerifyIterativeGVN checks that this optimization was applied
> 30:  * @requires vm.debug == true

Is that really required? Can't you get away with a `-XX:+IgnoreUnrecognizedVMOptions` if the problem is unrecognized VM options? Would be nice to know if it works in product too.

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

PR Review: https://git.openjdk.org/jdk/pull/26017#pullrequestreview-2970532176
PR Review Comment: https://git.openjdk.org/jdk/pull/26017#discussion_r2174630495