RFR: 8359602: Ideal optimizations depending on input type are missed because of missing notification mechanism from CCP [v2]

[Emanuel Peter]

On Mon, 30 Jun 2025 12:31:29 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
>> 
>> The...
>
> Benoît Maillard has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - 8359602: rename test
>  - 8359602: remove requires.debug=true and add -XX:+IgnoreUnrecognizedVMOptions flag

@benoitmaillard Very nice work, and great description :)

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

That may be a lot of work. Not sure if it is worth checking all of them now. @TobiHartmann how much should he invest in this now? An alternative is just tackling all the other cases later. What do you think?

@benoitmaillard One more open question for me: `raise_bottom_type` only sets the node internal `_type`. But in IGVN, we do not read from `_type` but `phase->type(in(2))`. Do you know when the `phase->type(in(2))` value changes? Is that also during CCP? Before or after the `_type` is modified?

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

PR Comment: https://git.openjdk.org/jdk/pull/26017#issuecomment-3019253499