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

[Benoît Maillard]

On Thu, 31 Jul 2025 13:28:14 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> 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.
>
> I think there is a misunderstanding here. I'm talking about node type, as in which C++ class is it, not type as abstract values for nodes. I could rename this struct then. Maybe HasNodeType? Or maybe `NodeClass`: one could see `NodeClass(&Node::is_Region)` that reads almost as "node class is Region". Open to ideas...
> 
> Also, in theory, it accepts any method of `Node` of type `bool()`. This could be used for something else. The idea was to make easy to say "I want a Node of type `IfNode` here". It's not that great to do with Opcode because of derived classes. I also considered something that would take any `Node -> bool` function, but that made the simple case harder. Instead of `HasType(&Node::is_If)`, I would have had to write something like `HasType([](const Node& n) { return n.is_If(); })`. Functional programming is possible in C++, but not quite syntactically elegant, and I think readability here is important. If such a need arises, I suggest to add a `UnaryPredicate` (or `NodePredicate` etc.) to do that. If the predicates are complicated enough, the bit of symbols needed for making a lambda doesn't matter so much.
> 
> As for your case, yes, we can add that in the future. It could be done with the UnaryPredicate I describe above, or with a more specific pattern that would work on types, and take a method `bool(Type::))()` or a function `bool(const Type&)` and the pattern would take care of finding the type and submitting it to the predicate. Not that it's a lot of work, but it allows to communicate more clearly the intention, in my opinion.

Yes of course, I got terribly confused, sorry for that. I agree that readability is important, and I would also keep it the way it is now. `HasNodeType` sounds pretty good in my opinion. Thanks for clarifying!

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

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