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

[Emanuel Peter]

On Mon, 8 Sep 2025 15:17:43 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   One more ResourceMark
>
> src/hotspot/share/opto/graphInvariants.cpp line 61:
> 
>> 59:  * and compositional to express complex structures from simple properties.
>> 60:  * For instance, we have a pattern for saying "the first input of the center match P" where P is another
>> 61:  * Pattern. We end up with trees of patterns matching the graph.
> 
> `the first input of the center match P` does not sound like a proper assertion.
> Some alternatives I could think of:
> - `match P on the first input of center` ok
> - `the first input of center must match P` ok
> - `match the first input of center with P` meh

Also: are we `check` ing or `match` ing? I would pick one consistently.

> src/hotspot/share/opto/graphInvariants.cpp line 234:
> 
>> 232:   bool check(const Node* center, Node_List& steps, GrowableArray<int>& path, stringStream& ss) const override {
>> 233:     if (!(center->*_type_check)()) {
>> 234:       ss.print_cr("Unexpected type: %s.", center->Name());
> 
> Is there a way we could say what we actually do expect? Not really, right? We'd need to do it via macro again.

Or we pass a string .. not nice but would work with the macro for `NodeClassIsAndBind`. Not sure what's best here.

> src/hotspot/share/opto/graphInvariants.cpp line 378:
> 
>> 376:     return CheckResult::VALID;
>> 377:   }
>> 378: };
> 
> I am wondering if it is really worth it to do the whole pattern matching approach, if we still have to write so much code.
> 
> There is a lot of boiler plate now, that has replaced the procedural code.
> 
> I'm just wondering if we are there yet, or if we need to find some way to make it more concise.
> Maybe we can do something like this:
> 
> return <something>
>        .applies_if(&Node::is_Phi)
>        .check([&]() { return PatternBasedCheck::check(center, reachable_cfg_nodes, steps, path, ss); })
>        .require(...)
>        .finish();
> 
> Just an idea. It would probably be lambda based again, which has its disadvantages.
> Maybe you have an even better idea.
> I'd just like to understand why the Pattern based approach is really super desirable, what are the advantages and disadvantages?

One advantage is definitively reporting. And it is still reasonably debuggable I think, my solution may be a little trickier that way.

I think there are multiple factors:
- Simple: fewer abstractions can be easier to read/debug.
- Concise: few lines of code.
- Reporting: nice output when rules fail.

> src/hotspot/share/opto/graphInvariants.cpp line 547:
> 
>> 545:         return CheckResult::FAILED;
>> 546:       }
>> 547:     }
> 
> If you do add `OuterStripMinedLoop`, make it a swich, and assert in the default case ;)

I just saw that you do the `OuterStripMinedLoop` below. But to capture the parallel structure it may still be good. And to capture possible future extension.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2330614759
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2330713629
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2330793156
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2330826144