RFR: 8359602: VerifyIterativeGVN fails with assert(!VerifyHashTableKeys || _hash_lock == 0) failed: remove node from hash table before modifying it
Benoît Maillard
bmaillard at openjdk.org
Mon Jun 30 11:56:41 UTC 2025
On Mon, 30 Jun 2025 09:25:35 GMT, Marc Chevalier <mchevalier 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`).
>>
>> 
>>
>> ### 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
>>
>> The...
>
> 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.
The entire `verify_PhaseIterGVN()` call hides behind a `NOT_PRODUCT` macro, so unless I am missing something fundemental here I think we really need the debug mode. You can find the call [here](https://github.com/benoitmaillard/jdk/blob/2ac5e725e472578618c5ab47425c52843dc119bb/src/hotspot/share/opto/phaseX.cpp#L1069).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26017#discussion_r2174896663
More information about the hotspot-compiler-dev
mailing list