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

[Marc Chevalier]

On Mon, 8 Sep 2025 15:48:48 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   One more ResourceMark
>
> src/hotspot/share/opto/graphInvariants.cpp line 215:
> 
>> 213:       ss.print_cr("Input at index %d is nullptr.", _which_input);
>> 214:       return false;
>> 215:     }
> 
> So we would never do `AtInput(0, ExpectNullptr())` for example?
> Fine with me, just an idea to consider ;)

No, we can't do that because every pattern must be applied on a center. `AtInput` moves the center. We cannot use a parametric pattern to check that a node around is not there: there would be no place to apply the parameter pattern. We can make `InputIsNull(int)` for that.

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

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

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