RFR: 8350864: C2: verify structural invariants of the Ideal graph

[Marc Chevalier]

On Thu, 17 Jul 2025 09:42:47 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

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

I understand the motivation, but I'm not sure what to do in every case. For instance, when the pattern is not so small (like strip mining), it's hard to associate the invariant with a single node: many are involved, and it's not really describable as the expectation of a single node. Of course, we could split the pattern into a lot of sub-patterns, centered each around each node type, but then, we lose the overview of the structure, and it becomes context-free (e.g. a IfFalse must have a CountedLoopEnd input only when it comes before a safepoint before a OuterStripMinedLoopEnd, but not in general).

Another problematic case is the control successor check that has special handling for some kinds, that could be relocated to the said node types, but for the general case, it simply tests whether the node is a CFG node. One could still do that in `Node`, but it's then not tied to a specific node type, and I feel it bloats the `Node` class (that is already not so small). And then, I fear it makes the invariant harder to read since it will be distributed across many node types: I would need to find overrides of `Verify`. When working on a given node, it may be easier to see what I need to guarantee (or change the invariant), but when working on something else, it makes harder to find the invariants I can actually rely on, because it could always be overridden in a derived class.

I also have a readability concern. Even if we sort them by node types, then we mix the implementation of all invariants of a given node in a single method, making it extra-hard to understand the big picture, and when looking for overrides, I will find some, but maybe they won't be about the invariant I'm interested in.

And a code/maintenance concern. If I have a default implementation of the control successor check in `Node`, among other such general checks, I'm tempted to override it in `IfNode` to accept having more than one successor, but then, how do I perform the other general checks that still hold? I can't call `Node::Verify` since it will enforce the wrong number of successor. I could put these checks in another method, and call it from `IfNode::Verify`, but it has other annoying consequences: if they are all in the same side method, I can't customize another of these checks in another node type; if each check is in its own method all called from `Node::Verify`, I need to repeat the call in the overrides of `Verify` for all the checks one will add in the future... Overall, it seems risky to maintain. We could also just call `Node::Verify` and have here some handling to skip some steps for some node types, but I feel like that defeats the point of having invariants closer from the type.

Overall, it seems to me that it's beneficial to move checks to the node types if:
- the pattern is small and clearly has a privileged node, so we won't be surprised by having the invariant implemented in another node type
- the pattern doesn't have special cases for sub-types.

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: some that are like this, implemented directly in the node, and some that are less local (not about a node, but about bigger shapes), or needs more special cases, and that we keep standalone. I don't think having two sources of invariants is a problem at all.

> on naming: IMO VerifyIdealGraph would clearly describe what the logic does, fits existing conventions well, and easy to find

Sure, fine with me! I'd be curious to see if somebody has other ideas.

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

PR Comment: https://git.openjdk.org/jdk/pull/26362#issuecomment-3083672692
PR Comment: https://git.openjdk.org/jdk/pull/26362#issuecomment-3083677141