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

Wed Sep 24 16:05:34 UTC 2025

On Tue, 9 Sep 2025 17:07:40 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:
> 
>   A better way to make them not debug-only, without very ad-hoc hacking

Thank you for working on this, @marc-chevalier! The verification of the graph structure is incredibly important and I am looking forward to writing my own invariants. I like that you put an emphasis on the quality of the error messages and the readability of the verification code, which is solved really well with the expression based matching you introduced. Still, I have a few comments and suggestions below.

Regarding other comments:

> > IMO it's better to have node-specific invariant checks co-located with corresponding node (as Node::verify() maybe?); it would make it clearer what are the expectations when changing the implementation.
>
> For instance, PhiArity would be a good candidate (about a special kind of node, no context needed, no exception). So, maybe a solution would be to split the checks in two sources.

I think this would be a good idea. Is this doable reasonably simply with your current centralized approach of adding checkers?

> Though it would be good to discuss a bit more how the patterns now look, especially if this becomes something that we do more widely eventually.

IMO, the focus of the pattern matching in this PR is more on providing the best error-reporting rather than a performant implementation that could eventually be used in IGVN. I think it would be detremental to conflate the two at this stage.

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

> 170:                 new AtSingleOutputOfType(&Node::is_IfTrue, new TruePattern()),
> 171:                 new AtSingleOutputOfType(&Node::is_IfFalse, new TruePattern()))) {
> 172:   }

Suggestion:

                new AtSingleOutputOfType(&Node::is_IfFalse, new TruePattern()))) {}

This is what you did on line 355 and I find it much more readable.

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

> 194:                     0,
> 195:                     NodeClassIsAndBind(Region, _region_node)))) {
> 196:   }

Suggestion:

                    NodeClassIsAndBind(Region, _region_node)))) {}

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

> 273: 
> 274: private:
> 275:   static void print_node_list(const Node_List& ctrl_succ, stringStream& ss) {

Perhaps this would be a good addition to `Node_List` as an analog to `Node::dump(suffix, mark, ss, dc)` instead of a private method of some verification class.

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

> 325:           for (uint i = 0; i < non_null_inputs.size(); ++i) {
> 326:             non_null_inputs.at(i)->dump("\n", false, &ss);
> 327:           }

That's `ControlSuccessor::print_node_list` from above...

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

> 450:     MultiBranchNode* mb = center->as_MultiBranch();
> 451:     if (mb->required_outcnt() < static_cast<int>(mb->outcnt())) {
> 452:       ss.print_cr("The required_outcnt of a MultiBranch node must be smaller than or equal to its outcnt. But required_outcnt=%d vs. outcnt=%d", mb->required_outcnt(), mb->outcnt());

Suggestion:

      ss.print_cr("The required_outcnt of a MultiBranch node must be smaller than or equal to its outcnt. But required_outcnt=%d vs. outcnt=%u", mb->required_outcnt(), mb->outcnt());

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

> 89:   bool run() const;
> 90: };
> 91: #endif

Suggestion:

#endif // !PRODUCT

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

Changes requested by mhaessig (Committer).

PR Review: https://git.openjdk.org/jdk/pull/26362#pullrequestreview-3234827746
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2376113619
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2376114238
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2376061890
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2376089020
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2376193017
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2355766897