RFR: 8350864: C2: verify structural invariants of the Ideal graph [v3]

[Emanuel Peter]

On Mon, 25 Aug 2025 14:07:02 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Benoît's comments
>
> src/hotspot/share/opto/graphInvariants.cpp line 270:
> 
>> 268:                 new HasNOutputs(2),
>> 269:                 new AtSingleOutputOfType(&Node::is_IfTrue, new True()),
>> 270:                 new AtSingleOutputOfType(&Node::is_IfFalse, new True()))) {
> 
> I would suggest that you append the word `Pattern` to all `Patterns` - at least in most cases this will make it a bit easier to see what you have at the use-site. I'm looking at `new True()` and wonder what might be passed here... if it was called `TruePattern`, it would be immediately clear.

You could leave a comment at `True(Pattern)` that it is (often) used as the terminal pattern, at the end of a branch / search.

> src/hotspot/share/opto/graphInvariants.cpp line 287:
> 
>> 285:       }
>> 286:     }
>> 287:     return r;
> 
> Also this could probably be handled with a pattern wrapping mechanism, right?
> `FailOnlyForLiveNodes( <the pattern from above> )`

I'm just suggesting it in case you need to do this sort of special-casing elsewhere too ;)

> src/hotspot/share/opto/graphInvariants.cpp line 301:
> 
>> 299:                     And::make(
>> 300:                         new NodeClass(&Node::is_Region),
>> 301:                         new Bind(region_node))))) {
> 
> This sort of binding is kinda cool! Never thought of it before. Could be really cool for general pattern matching.
> We would have to find a solution if there would be multiple bindings though ... I think that's not possible with your patterns, right? Is that a fundamental constraint?

What would be extra cool / funky:
If we could somehow already cast the `Bind` variable to `Region`. Could be tricky.
Doing this `is_Region and bind` could be a very common idiom, so very useful.

> src/hotspot/share/opto/graphInvariants.cpp line 417:
> 
>> 415:     if (self == nullptr) {
>> 416:       // Must be a copy Region
>> 417:       Node_List non_null_inputs;
> 
> ResouceMark?

Is it worth it to do the allocation, if in most cases we just expect 1 non-null?
Why not count non-nulls, and if we find more than one, traverse again over the Region, and filter and dump them?

> src/hotspot/share/opto/graphInvariants.cpp line 452:
> 
>> 450:                             And::make(
>> 451:                                 new NodeClass(&Node::is_BaseCountedLoopEnd),
>> 452:                                 new Bind(counted_loop))))))) {}
> 
> Ah, another check and Bind! Why not allow `Bind<BaseCountedLoopEndNode*>`, so we can bind it with the cast?

And I would call it `counted_loop_end`.

> src/hotspot/share/opto/graphInvariants.cpp line 469:
> 
>> 467:     assert(counted_loop != nullptr, "sanity");
>> 468:     if (is_long) {
>> 469:       if (counted_loop->is_CountedLoopEnd()) {
> 
> Sounds like head/tail confusion here. Call it `counted_loop_end`.

Also: I would invert the check to `!counted_loop_end->is_LongCountedLoopEnd()`. Because you expect it to be a long end here. Subjective.

> src/hotspot/share/opto/phaseX.hpp line 615:
> 
>> 613:   Node* _verify_window[_verify_window_size];
>> 614:   void verify_step(Node* n);
>> 615:   GraphInvariantChecker* _invariant_checker;
> 
> Why do you allocate it separately, and not have it in-place?

Is there only a single PhaseIterGVN per compilation? I forgot. An alternative would be to allocate it at the level of the compilation.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2298220598
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2298241305
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2298251176
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2298295335
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2298305594
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2298312279
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2298153277