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

[Benoît Maillard]

On Wed, 30 Jul 2025 08:27:37 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> Some crashes are consequences of earlier misshaped ideal graphs, which could be detected earlier, closer to the source, before the possibly many transformations that lead to the crash.
>> 
>> Let's verify that the ideal graph is well-shaped earlier then! I propose here such a feature. This runs after IGVN, because at this point, the graph, should be cleaned up for any weirdness happening earlier or during IGVN.
>> 
>> This feature is enabled with the develop flag `VerifyIdealStructuralInvariants`. Open to renaming. No problem with me! This feature is only available in debug builds, and most of the code is even not compiled in product, since it uses some debug-only functions, such as `Node::dump` or `Node::Name`.
>> 
>> For now, only local checks are implemented: they are checks that only look at a node and its neighborhood, wherever it happens in the graph. Typically: under a `If` node, we have a `IfTrue` and a `IfFalse`. To ease development, each check is implemented in its own class, independently of the others. Nevertheless, one needs to do always the same kind of things: checking there is an output of such type, checking there is N inputs, that the k-th input has such type... To ease writing such checks, in a readable way, and in a less error-prone way than pile of copy-pasted code that manually traverse the graph, I propose a set of compositional helpers to write patterns that can be matched against the ideal graph. Since these patterns are... patterns, so not related to a specific graph, they can be allocated once and forever. When used, one provides the node (called center) around which one want to check if the pattern holds.
>> 
>> On top of making the description of pattern easier, these helpers allows nice printing in case of error, by showing the path from the center to the violating node. For instance (made up for the purpose of showing the formatting), a violation with a path climbing only inputs:
>> 
>> 1 failure for node
>>  211  OuterStripMinedLoopEnd  === 215 39  [[ 212 198 ]] P=0,948966, C=23799,000000
>> At node
>>     209  CountedLoopEnd  === 182 208  [[ 210 197 ]] [lt] P=0,948966, C=23799,000000 !orig=[196] !jvms: StringLatin1::equals @ bci:12 (line 100)
>>   From path:
>>     [center] 211  OuterStripMinedLoopEnd  === 215 39  [[ 212 198 ]] P=0,948966, C=23799,000000
>>       <-(0)- 215  SafePoint  === 210 1 7 1 1 216 37 54 185  [[ 211 ]]  SafePoint  !orig=186 !jvms: StringLatin1::equals @ bci:29 (line 100)
>>       <-(0)- 210  IfFalse  === 209  [[ 21...
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Rename flag as suggested

Great work, and great explanation as well! The invariants that are already implemented seem quite useful already, and it seems there is a lot of potential.

Having recently worked on a few missed optimizations related to `PhaseIterGVN::add_users_of_use_to_worklist`, I agree that it would be interesting to use such patterns for automatic notifications. The way I see it, we would need to somehow "reverse" the patterns, as they would be expressed from the point of view of the node on which the optimizations is applied, and would require notification when dependencies changes. Probably quite non-trivial, but interesting nonetheless. 

I only have a few basic remarks/questions.

src/hotspot/share/opto/graphInvariants.cpp line 181:

> 179:   AtInput(uint which_input, const Pattern* pattern) : _which_input(which_input), _pattern(pattern) {}
> 180:   bool check(const Node* center, Node_List& steps, GrowableArray<int>& path, stringStream& ss) const override {
> 181:     assert(_which_input < center->req(), "First check the input number");

This really a detail, but I would use something more explicit:

Suggestion:

    assert(_which_input < center->req(), "Input number is out of range");

src/hotspot/share/opto/graphInvariants.cpp line 197:

> 195: };
> 196: 
> 197: struct HasType : Pattern {

Could we make it slightly more general and accept any predicate on the type? From a previous PR that I worked on I remember that for example for `ModINode` if it has no control input then its divisor input should never be `0`. Maybe this is the kind of properties we could check in the future. This is just a random idea, feel free to ignore.

src/hotspot/share/opto/graphInvariants.hpp line 32:

> 30: 
> 31: // An invariant that needs only a local view of the graph, around a given node.
> 32: class LocalGraphInvariant : public ResourceObj {

Can't we put the whole definition behind a `#ifndef PRODUCT` check? It seems there are other instances where it is done with classes, such as `VTrace`. Or is there a reason not to?

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

PR Review: https://git.openjdk.org/jdk/pull/26362#pullrequestreview-3074918758
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2245262469
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2245282832
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2245044927