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

Wed Nov 19 10:59:41 UTC 2025

On Wed, 19 Nov 2025 08:55:07 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.
>> 
>> # 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 and see if something change".
>> 
>> To me, the surprising fact was that the intersection
>> 
>> java/lang/Object * (speculative=compiler/igvn/ClashingSpeculativeTypePhiNode$C2:exact *)
>> /\
>> _type=java/lang/Objec...
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix bug number

Thanks for the update. I now had a closer look, and got a little confused in multiple places. I'm not sure I have the right mental model yet.

I was also a little surprised by the fact that we do have inconsistent profiling. But maybe that should not be surprising? That's where a closer look at the reproducer would help me follow, and annotations in the test itself would be fantastic ;)

I have not yet looked at all the debug code in-depth, as I'd like to first understand the rest.

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

> 1355:   // In rare cases, `_type` and `t` have incompatible opinion on speculative type, resulting into a too small intersection
> 1356:   // (such as AnyNull), which is removed in cleanup_speculative. From that `ft` has empty speculative type. After the end
> 1357:   // of the current `Value` call, `ft` (that is returned) is becoming `_type`. If verification happens then, `t` would be the

Suggestion:

  // of the current `Value` call, `_type` is assigned the value of `ft`. If verification happens then, `t` would be the

"becoming" is a bit ambiguous here, I first thought it meant `ft = _type`, then realized one could also read it as `_type = ft`. Maybe you have an even better way to express it.

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

> 1358:   // same (union of input types), but the new `_type` has now no speculative type, the result of `t->filter_speculative(_type)`
> 1359:   // has the speculative type of `t` (if it's not removed because e.g. the resulting type is exact and non null) and not empty
> 1360:   // (like the previously returned type). In such a case, doing the filtering one time more allows to reach a fixpoint.

>  From that `ft` has empty speculative type

I'm not very familiar with speculative types. Does "empty speculative" == TOP speculative type? Or rather "no speculative type", which essencially means it is BOTTOM type?

Because then if we filter x with TOP we should still get TOP, but if we filter with BOTTOM we get x. And that would fit better with your statement later on:

> but the new `_type` has now no speculative type, the result of `t->filter_speculative(_type)` has the speculative type of `t`

Can you clarify please for my understanding? :)

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

> 1363:     ft = t->filter_speculative(first_ft);
> 1364: #ifdef ASSERT
> 1365:     // The following logic has been moved into TypeOopPtr::filter.

Why does this mean? What logic are you referring to? The one here? But then you say it was moved to TypeOopPtr::filter ...but it is still here? Can you clarify?

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

> 1365:     // The following logic has been moved into TypeOopPtr::filter.
> 1366:     const Type* jt = t->join_speculative(first_ft);
> 1367:     if (jt->empty()) {           // Emptied out???

Suggestion:

    if (jt->empty()) {

Comment seems redundant.

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

> 1373:     else {
> 1374: 
> 1375:       if (jt != ft && jt->base() == ft->base()) {

Formatting looks a bit funny here.

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

PR Review: https://git.openjdk.org/jdk/pull/28331#pullrequestreview-3481920110
PR Review Comment: https://git.openjdk.org/jdk/pull/28331#discussion_r2541392783
PR Review Comment: https://git.openjdk.org/jdk/pull/28331#discussion_r2541429817
PR Review Comment: https://git.openjdk.org/jdk/pull/28331#discussion_r2541495880
PR Review Comment: https://git.openjdk.org/jdk/pull/28331#discussion_r2541500946
PR Review Comment: https://git.openjdk.org/jdk/pull/28331#discussion_r2541492662