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

[Tobias Hartmann]

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...

I agree with Marc, really nice write-up! The fix looks good to me.

Did you check if this allows enabling any of the other disabled verifications from [JDK-8347273](https://bugs.openjdk.org/browse/JDK-8347273)? 

Please change the title of this issue to something more descriptive.

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

> 33:  *      -XX:CompileCommand=compileonly,compiler.c2.TestPropagateTypeRefinementToIGVN::test
> 34:  *      compiler.c2.TestPropagateTypeRefinementToIGVN
> 35:  * @run main compiler.c2.TestPropagateTypeRefinementToIGVN

The test name is a bit too generic, maybe change to `TestModControlFoldedAfterCCP` or something.

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

Changes requested by thartmann (Reviewer).

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