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

[Emanuel Peter]

On Tue, 9 Sep 2025 07:40:23 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> src/hotspot/share/opto/graphInvariants.cpp line 222:
>> 
>>> 220:     }
>>> 221:     return result;
>>> 222:   }
>> 
>> Would this not read better?
>> Suggestion:
>> 
>>     bool success = _pattern->check(center->in(_which_input), state);
>>     if (!success) {
>>       state.trace_failure_path(center, _which_input);
>>     }
>>     return success;
>>   }
>
> I don't think it's terrible, but I don't think it's much better. If I know the code and that we want to add the new points in the path, and I'll read it as such either way. Or I don't know the code, but I know the types of `steps` and `path`, and I know what `push` does, while a custom type with custom methods has an higher learning cost. So to me, it's pretty equivalent.

To me, it was a high overhead having to find out where the `steps` `path` and `ss` were defined. If I know it is some state, I can quickly go to the definition, and see what it is all about.
You can also call the method `state.push_to_paths_and_steps(center, _which_input)`.

>> src/hotspot/share/opto/graphInvariants.cpp line 239:
>> 
>>> 237:     return true;
>>> 238:   }
>>> 239:   bool (Node::*_type_check)() const;
>> 
>> Suggestion:
>> 
>> private:
>>   bool (Node::*_type_check)() const;
>> 
>> I would also suggest that you use a `typedef` here.
>> Something like:
>> `typedef bool (Node::*TypeCheckMethod)() const;`
>> Then you can write
>> Suggestion:
>> 
>> public:
>>   const TypeCheckMethod _type_check;
>
> Again, is it really better? If I know the code, I know what it needs, however I express it. If I don't know the code, looking at the signature won't be enough, I'll need to look up one level deeper the definition. Not sure it's a win.

I think it is a matter of taste. I don't personally like the C++ way of expressing pointer types. But I can get used to it.

>> src/hotspot/share/opto/graphInvariants.hpp line 73:
>> 
>>> 71:    * In addition, if the check fails, it must write its error message in [ss].
>>> 72:    *
>>> 73:    * If the check succeeds or is not applicable, [steps], [path] and [ss] must be untouched.
>> 
>> I wonder if we should not have some object that represents these 3 args. You pass them everywhere, and they seem to be a unit. And they have invariants that we may want to check.
>> You could for example enforce that steps and path are in synch just by only providing the access methods that allow it.
>> What do you think?
>
> `steps` and `path` can make sense. I don't think it makes sense for `ss` because we just fill it from `steps` and `path` at some point, it doesn't really evolve with. If you like it, I won't fight, but is it worth it? It seems like more ad-hoc types to be aware of for simplifying the code a little and real benefits but not big benefits imo.

Let's ask @chhagedorn . He might have a good idea too here ;)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2332506238
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2332490006
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2332509836