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

[Marc Chevalier]

On Mon, 8 Sep 2025 16:42:32 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> 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.

I could have wrote this without pattern at all, but I also want to make more example of differently complex usage of patterns. Writing it without patterns at all would be pretty similar to me.

I think the boilerplate has to exist somewhere. It's not nice to read, it's long, but it's (actually) simple. If we hide it somewhere, it's nicer to read and gives an impression of easier to understand, but harder to actually understand when something goes wrong. No strong opinion.

>> 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.

I don't understand.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2332373118
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2332384303