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

[Manuel Hässig]

On Wed, 28 Jan 2026 17:05:55 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 incrementally with one additional commit since the last revision:
> 
>   Change condition structure

I still have a few nits. Apart from those this looks good.

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

> 1182:     // false conservatively, and later it can determine that it is indeed true. Loops with
> 1183:     // Region heads can lead to giving up, whereas LoopNodes can be skipped easier, and
> 1184:     // so the traversal becomes more powerful. This is difficult to remidy, we would have

Suggestion:

    // so the traversal becomes more powerful. This is difficult to remedy, we would have

Small typo I just noticed

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

> 1205:     assert(_phase == PhaseValuesType::ccp, "Unexpected phase identifier");
> 1206:     assert(false, "PhaseCCP not at fixpoint: analysis result may be unsound for %s", n->Name());
> 1207:   }

Suggestion:


  switch (_phase) {
    case PhaseValuesType::iter_gvn:
      assert(false, "Missed Value optimization opportunity in PhaseIterGVN for %s",n->Name());
      break;
    case PhaseValuesType::ccp:
      assert(false, "PhaseCCP not at fixpoint: analysisresult may be unsound for %s", n->Name());
      break;
    default:
      assert(false, "Unexpected phase");
      break;
  }

With an enum, a switch case seems safer.

src/hotspot/share/opto/phaseX.hpp line 265:

> 263:   }
> 264:   NOT_PRODUCT(~PhaseValues();)
> 265:   PhaseIterGVN* is_IterGVN() { return (_phase >= PhaseValuesType::iter_gvn) ? (PhaseIterGVN*)this : nullptr; }

Suggestion:

  PhaseIterGVN* is_IterGVN() { return (_phase == PhaseValuesType::iter_gvn || _phase == PhaseValuesType::ccp) ? static_cast<PhaseIterGVN*>(this) : nullptr; }

I find relying on the enum order quite error prone. Also, we should probably prefer a static cast.

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

Changes requested by mhaessig (Committer).

PR Review: https://git.openjdk.org/jdk/pull/28295#pullrequestreview-3722259656
PR Review Comment: https://git.openjdk.org/jdk/pull/28295#discussion_r2741297466
PR Review Comment: https://git.openjdk.org/jdk/pull/28295#discussion_r2741306347
PR Review Comment: https://git.openjdk.org/jdk/pull/28295#discussion_r2741288073