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

[Marc Chevalier]

On Mon, 25 Aug 2025 14:14:41 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Benoît's comments
>
> src/hotspot/share/opto/graphInvariants.cpp line 279:
> 
>> 277:       return CheckResult::NOT_APPLICABLE;
>> 278:     }
>> 279:     CheckResult r = PatternBasedCheck::check(center, reachable_cfg_nodes, steps, path, ss);
> 
> Could this not be solved with a `OrPattern`?
> 
> Or::make( <not is_If ...,
>   <the And::make from above>
> )
> 
> Not sure that's worth it...

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.

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

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