RFR: 8371716: C2: Phi node fails Value()'s verification when speculative types clash [v4]

[Emanuel Peter]

On Thu, 27 Nov 2025 09:46:23 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> This bug was originally found and reported as a Valhalla problem. It quickly became apparent it has no reason to be Valhalla-specific, while I couldn't prove so. Roland managed to make a mainline reproducer. The explanation details my Valhalla investigation, but it has nothing to do with value classes anyway.
>> 
>> The proposed solution seems somewhat controversial. See https://github.com/openjdk/valhalla/pull/1717 for some previous discussion. Before polishing the PR, I'd like to reach an agreement on the way to go.
>> 
>> # Analysis
>> ## Obervationally
>> ### IGVN
>> During IGVN, in `PhiNode::Value`, a `PhiNode` has 2 inputs. Their types are:
>> 
>> in(1): java/lang/Object * (speculative=TestSpeculativeTypes$C2:NotNull:exact * (inline_depth=3))
>> in(2): null
>> 
>> We compute the join (HS' meet):
>> https://github.com/openjdk/jdk/blob/09b25cd0a24a4eaddce49917d958adc667ab5465/src/hotspot/share/opto/cfgnode.cpp#L1299-L1306
>> 
>> t=java/lang/Object * (speculative=compiler/igvn/ClashingSpeculativeTypePhiNode$C2:exact *)
>> 
>> But the current `_type` (of the `PhiNode` as a `TypeNode`) is
>> 
>> _type=java/lang/Object * (speculative=compiler/igvn/ClashingSpeculativeTypePhiNode$C1:exact *)
>> 
>> We filter `t` by `_type`
>> https://github.com/openjdk/jdk/blob/09b25cd0a24a4eaddce49917d958adc667ab5465/src/hotspot/share/opto/cfgnode.cpp#L1321
>> and we get
>> 
>> ft=java/lang/Object *
>> 
>> which is what we return. After the end of `Value`, the returned becomes the new `PhiNode`'s `_type`.
>> https://github.com/openjdk/jdk/blob/09b25cd0a24a4eaddce49917d958adc667ab5465/src/hotspot/share/opto/phaseX.cpp#L2150-L2164
>> and
>> https://github.com/openjdk/jdk/blob/09b25cd0a24a4eaddce49917d958adc667ab5465/src/hotspot/share/opto/node.cpp#L1117-L1123
>> 
>> 
>> ### Verification
>> On verification, `in(1)`, `in(2)` have the same value, so does `t`. But this time
>> 
>> _type=java/lang/Object *
>> 
>> and so after filtering `t` by (new) `_type` and we get
>> 
>> ft=java/lang/Object * (speculative=compiler/igvn/ClashingSpeculativeTypePhiNode$C2:exact *)
>> 
>> which is retuned. Verification gets angry because the new `ft` is not the same as the previous one.
>> 
>> ## But why?!
>> ### Details on type computation
>> In short, we are doing
>> 
>> t = typeof(in(1)) / typeof(in(2))
>> ft  = t /\ _type (* IGVN *)
>> ft' = t /\ ft    (* Verification *)
>> 
>> and observing that `ft != ft'`. It seems our lattice doesn't ensure `(a /\ b) /\ b = a /\ b` which is problematic for this kind of verfication that will just "try again...
>
> Marc Chevalier has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - test
>  - Merge branch 'master' into JDK-8371716
>  - More test
>  - IgnoreUnrecognizedVMOptions
>  - Fix bug number
>  - Filter twice

I think this solution is reasonable, and we discussed different options.

Thanks @marc-chevalier for working on this.

And thanks especially for the extra descriptions in the tests. It was super fascinating to see how we get to the inconsistent speculative types :)

@merykitty It would be really interesting to see if the "widening" we do when we go above centerline with speculative types really makes sense. I'm sure there are micro benchmarks that would benefit from the extra speculation, and others that would suffer from it. I suppose we would have to make the changes and run larger benchmarks. Is that something you would want to look into?

src/hotspot/share/opto/cfgnode.cpp line 1377:

> 1375:     stringStream ss;
> 1376: 
> 1377:     ss.print("At node:\n");

Suggestion:

    ss.print_cr("At node:");

src/hotspot/share/opto/cfgnode.cpp line 1382:

> 1380:     for (uint i = 1; i < req(); ++i) {
> 1381:       ss.print("in(%d): ", i);
> 1382:       if (r->in(i) && phase->type(r->in(i)) == Type::CONTROL) {

Suggestion:

      if (r->in(i) != nullptr && phase->type(r->in(i)) == Type::CONTROL) {

No implicit null check.

src/hotspot/share/opto/cfgnode.cpp line 1407:

> 1405:     assert(false, "computed type would not pass verification");
> 1406:   }
> 1407: #endif

You could consider moving this to a seperate verification method, but up to you.

It is also very verbose. I wonder if it really makes sense to have that much code. But again: up to you.

You could also make your dump less verbose, and then format it directly into the assert. The benefit would be that if a fuzzer finds such a failure we would see more directly what's going on.

test/hotspot/jtreg/compiler/igvn/ClashingSpeculativeTypePhiNode.java line 57:

> 55:  *                   compiler.igvn.ClashingSpeculativeTypePhiNode
> 56:  *
> 57:  * @run main compiler.igvn.ClashingSpeculativeTypePhiNode

Suggestion:

 * @run main ${test.main.class}

The new JTREG version allows using this template. It would prevent issues where you wrongly copy from another file and forget to fix up the class name ;)

You can apply it also to the compilecommands.

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/28331#pullrequestreview-3514828452
PR Review Comment: https://git.openjdk.org/jdk/pull/28331#discussion_r2568358477
PR Review Comment: https://git.openjdk.org/jdk/pull/28331#discussion_r2568359469
PR Review Comment: https://git.openjdk.org/jdk/pull/28331#discussion_r2568357864
PR Review Comment: https://git.openjdk.org/jdk/pull/28331#discussion_r2568256187