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

[Marc Chevalier]

On Fri, 5 Sep 2025 07:36:34 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> I understand that OrPatterns are tempting! I also thought about it, it's naturally the dual of `And`. At this point, they are not actually a good idea.
>> 
>> First, they cannot provide good reporting. When an `And` is failing, we can at least blame the first thing that fails: "I followed this path, I expected to find 5 inputs (for instance), there are only 2!". With `Or` we would get that and... maybe it's fine? Maybe not? Depends on the next branches, and if it ends up failing, how to provide a good message?
>> 
>> Also, they cause a mess with binding. If a branch contains a `Bind`, one cannot know which branch matched and whether the content of the `Node` pointer given to `Bind` is trustworthy. We can't even rely on a test whether the pointer was set because the execution of a branch might find a `Bind` first, run it, assign the pointer and later fail, and then the `Bind` is not to use. This is a common problem with pattern matching in functional programming: the same bindings must appear (with same types) on each branch of or-patterns. But we have no such mechanisms to enforce that yet, and it seems like setting a trap for future us.
>> 
>> There is also relatively few use cases, and that would not profit a lot from a `Or` pattern. Maybe in the future, we will have more interesting usecases and we will see how to address these issues. But for now, I think we should not include it for now rather than making a bad choice.
>> 
>> By the way, I think something that has more future than `Or` is rather a case analysis: `IfThenElse(CondtionPattern, TrueBranchPattern, FalseBranchPattern)` if CondtionPattern is true, then we try to match TrueBranchPattern, otherwise FalseBranchPattern. This is better for reporting since we know which branch to we expect to be true, and so to blame (assuming we don't blame CondtionPattern, but we can include that in the message possibly). This still has the binding consistency issue, but more boilerplate could help (querying the set of pointers that would be set in each branch with helping methods...). Yet, let's wait and see.
>
> Hmm, yes. I did later on think about binding.
> If we ever use pattern matching for IGVN optimizations, we need to be able to do or-like patterns, maybe even over 2, 3, ...n many branches. And then bind to something.
> 
> And you are right, reporting could also be an issue.
> Maybe there could be some kind of reporting still though: we could evaluate both branches and report where each fails.
> 
> I saw multiple uses, so maybe at some point an Or could be justified. But maybe not yet.
> 
> I think this is an interesting thread, so a shame you closed it as "resolved" ;)

I think using Or patterns for recognizing patterns, and not for enforcing them is nicer since there is no need reporting problem. We can add that when we are there.

>> Everything will run under `GraphInvariantChecker::run()` that has a `ResouceMark`. I'm not sure, but my guess is that it's not worth keeping entering and leaving resource marks for relatively short lists? At the very list, everything will be released at the end of the whole check. I can still add one here if you think it's better.
>
> I would do it defensively. Might save us from out-of-memory later on with higher tiers, and it could also make things faster: i.e. we might avoid timeouts, just because we need less memory. I don't have the overview how large these are and how many you'd create, so maybe it is unnecessary. Up to you.

I will then! I've run this flag to tier3 + stress without OOM, so it's not too terrible, but yeah, we never know what kind of memory usage will come!

>> I surely hope not!
>
> Then I would assert that it has exactly 1 input instead ;)

Yes, I've changed it already (is GH laggy?...)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2324352658
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2324363672
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2324367309