RFR: 8371536: C2: VerifyIterativeGVN should assert on first detected failure [v6]

[Emanuel Peter]

On Thu, 18 Dec 2025 12:08:56 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

>> This PR introduces changes in the detection of missing IGVN optimizations. As explained in the JBS issue description, when `-XX:VerifyIterativeGVN` was introduced, it was helpful to list all the missing optimizations. Such failures occur less frequently now, and the focus has changed to being able to debug such failure quickly and identifying similar or related failures during bug triaging.
>> 
>> In summary, this PR brings the following changes:
>> - Assert at the first verification failure in `verify_Optimize` instead of attemtping to process all the nodes in the graph. This makes the output easier to parse, and also decreases the overhead of getting to the actual optimization site with a debugger.
>> - Avoid confusing `Need to remove from hash before changing edges` assert messages by removing the verified node from the hash table before attempting to optimize the node in question.
>> - Provide the failure reason (Ideal, Identity or Value) and the node name in the assert message itself to facilitate identifying related failures in the testing infrastructure during bug triaging.
>> 
>> ### Example outputs
>> #### [JDK-8371534: C2: Missed Ideal optimization opportunity with AndL and URShiftL ](https://bugs.openjdk.org/browse/JDK-8371534)
>> Before the change, we would get two missed optimizations (the second one is only a consequence of the first one). After the change, we only get the first one, which is the one that actually needs to be fixed. We also get the name of the node in the assert message.
>> <details>
>> <summary>Before</summary>
>> 
>> 
>> Missed Ideal optimization (can_reshape=false):
>> The node was replaced by Ideal.
>> Old node:
>> dist dump
>> ---------------------------------------------
>>    1   22  ConI  === 0  [[ 70 81 70 290 81 76 32 37 37 43 48 48 54 59 59 65 336 ]]  #int:1
>>    1  297  AndL  === _ 298 21  [[ 290 ]]  !orig=[236],[193] !jvms: TestMaskAndRShiftReorder::testURShiftL @ bci:42 (line 81)
>>    0  290  URShiftL  === _ 297 22  [[ 299 ]]  !orig=[231],[194] !jvms: TestMaskAndRShiftReorder::testURShiftL @ bci:46 (line 82)
>> The result after Ideal:
>> dist dump
>> ---------------------------------------------
>>    1  337  ConL  === 0  [[ 338 ]]  #long:-9
>>    1  336  URShiftL  === _ 298 22  [[ 338 ]] 
>>    0  338  AndL  === _ 336 337  [[ ]] 
>> 
>> 
>> Missed Ideal optimization (can_reshape=true):
>> The node was replaced by Ideal.
>> Old node:
>> dist dump
>> ---------------------------------------------
>>    1   22  ConI  === 0  [[ 70 81 70 290 81 76...
>
> Benoît Maillard has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:
> 
>  - Remove trailing whitespace
>  - Merge branch 'master' into JDK-8371536
>  - More style
>  - Style and comments
>  - Merge branch 'master' into JDK-8371536
>  - Assert directly in the verify methods
>  - Add comment for _table.hash_delete(n)
>  - Change assert to print only the cause and the node name
>    
>    Bring back old comment
>    
>    Wording
>  - Assert at first failure
>  - Remove node from hash table before calling Ideal in verification

Changes requested by epeter (Reviewer).

src/hotspot/share/opto/phaseX.cpp line 1090:

> 1088:       // We should either make sure that this node is properly added back to the IGVN worklist
> 1089:       // in PhaseIterGVN::add_users_to_worklist to update it again or add an exception
> 1090:       // in the verification code above if that is not possible for some reason (like Load nodes).

You moved the comment. It used to make sense to refer to "exceptions above". Now the exceptions are inside the methods, relative to the new place of the comment ;)

src/hotspot/share/opto/phaseX.cpp line 1202:

> 1200:   tty->print_cr("%s", ss.as_string());
> 1201: 
> 1202:   assert(false, "Missed Value optimization opportunity in PhaseIterGVN for %s", n->Name());

What if it gets called during CCP? Then it is not just a missed opportunity, but possibly a correctness problem.

I wonder if we should have different assert messages here. We could even just pass a string into the method, either `IGVN` or `CCP`.

What do you think?

src/hotspot/share/opto/phaseX.cpp line 1839:

> 1837:   // return and finally hit the assert in PhaseIterGVN::verify_optimize to get
> 1838:   // a more meaningful message
> 1839:   _table.hash_delete(n);

Looks like an unrelated change. Why are you adding it now?

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

PR Review: https://git.openjdk.org/jdk/pull/28295#pullrequestreview-3593092023
PR Review Comment: https://git.openjdk.org/jdk/pull/28295#discussion_r2631174105
PR Review Comment: https://git.openjdk.org/jdk/pull/28295#discussion_r2631192720
PR Review Comment: https://git.openjdk.org/jdk/pull/28295#discussion_r2631179338