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

[marc-chevalier]

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 openjdk/valhalla#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):

jdk/src/hotspot/share/opto/cfgnode.cpp

Lines 1299 to 1306 in 09b25cd

	const Type *t = Type::TOP; // Merged type starting value
	for (uint i = 1; i < req(); ++i) {// For all paths in
	// Reachable control path?
	if (r->in(i) && phase->type(r->in(i)) == Type::CONTROL) {
	const Type* ti = phase->type(in(i));
	t = t->meet_speculative(ti);
	}
	}

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

jdk/src/hotspot/share/opto/cfgnode.cpp

Line 1321 in 09b25cd

const Type* ft = t->filter_speculative(_type); // Worst case type

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.

jdk/src/hotspot/share/opto/phaseX.cpp

Lines 2150 to 2164 in 09b25cd

	const Type* t = k->Value(this);
	assert(t != nullptr, "value sanity");
	// Since I just called 'Value' to compute the set of run-time values
	// for this Node, and 'Value' is non-local (and therefore expensive) I'll
	// cache Value. Later requests for the local phase->type of this Node can
	// use the cached Value instead of suffering with 'bottom_type'.
	if (type_or_null(k) != t) {
	#ifndef PRODUCT
	inc_new_values();
	set_progress();
	#endif
	set_type(k, t);
	// If k is a TypeNode, capture any more-precise type permanently into Node
	k->raise_bottom_type(t);

and

jdk/src/hotspot/share/opto/node.cpp

Lines 1117 to 1123 in 09b25cd

	void Node::raise_bottom_type(const Type* new_type) {
	if (is_Type()) {
	TypeNode *n = this->as_Type();
	if (VerifyAliases) {
	assert(new_type->higher_equal_speculative(n->type()), "new type must refine old type");
	}
	n->set_type(new_type);

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/Object * (speculative=compiler/igvn/ClashingSpeculativeTypePhiNode$C1:exact *)
~>
java/lang/Object *

What happened to the speculative type? Both C1 and C2 are inheriting Object. So the code correctly find that the intersection of these speculative type is

ava/lang/Object:AnyNull * (flat in array),iid=top

The interesting part is that it's AnyNull: indeed, what else is a C1 and C2 at the same time? And then, above_centerline decides it's not useful enough (too precise, too close from HS' top/normal bottom) and remove the speculative type.

jdk/src/hotspot/share/opto/type.cpp

Lines 2747 to 2749 in 09b25cd

	if (above_centerline(speculative()->ptr())) {
	return no_spec;
	}

But on the verification run, java/lang/Object * (speculative=compiler/igvn/ClashingSpeculativeTypePhiNode$C2:exact *) is intersected with the speculative type of java/lang/Object *, which is unknown (HS' bottom/normal top), so we are simply getting C2. If we did not discard AnyNull using above_centerline, we would have the intersection of C2 and AnyNull, giving AnyNull, which is indeed stable.

Reproducing

In Valhalla

This crash is quite rare because:

1. it needs a specific speculative type setup, which depends heavily on timing
2. if PhiNode::Value is called a second time, it will stabilize the _type field before verification.

To limitate the influence of 2., I've tested with an additional assert that would immediately do

const Type* ft_ = t->filter_speculative(ft);

in PhiNode::Value and compare ft and ft_. Indeed, we are never sure a run of Value is not the last one: it should always be legal to stop anywhere (even if in a particular case), it was going to run further.

With this extra check, the crash a bit more common, but still pretty rare. Tests that have been witness to crash then at least once:

• compiler/valhalla/inlinetypes/TestCallingConvention.java
• compiler/valhalla/inlinetypes/TestIntrinsics.java
• compiler/valhalla/inlinetypes/TestArrays.java
• compiler/valhalla/inlinetypes/TestBasicFunctionality.java

All in compiler/valhalla/inlinetypes while I was also testing with mainline tests. Suspicious, uh.

In mainline

With the aforementioned extra check, I've tried to see if it could happen on mainline since the involved code seems not to be valhalla-specific. Yet, nothing failed.

Fortunately, Roland crafted an example that reproduces in mainline (and Valhalla)! You'll find it as a test here.

Fixing?

I think changing the type system would be quite risky: it is all over the place. Also, fixing would require not to drop the speculative type when above_centerline. This might not be desirable. On top of the complexity and the associated risk, a too specific speculative type is rather useless. If we keep the too specific type around, we probably should ignore it where we make use of it. That's distributing the effort and open the door to inconsistencies. If we should ignore it, we might just as well drop it immediately. It is also dubious whether ordering requirement are meaningful for speculative type: they are not sound or complete. Moreover, one could argue that in the abstract, we don't even need a lattice or anything like that. A single poset whose functions are sound approximation of the concrete is enough. It is not uncommon that in the abstract world that a /\ b is not smaller than a.

For instance, in the co-interval domain (the whole universe E minus an interval), if we take a = E \ [1, 2] and b = E \ [3, 4], the concrete intersection would be E \ [1, 2] \ [3, 4] which is not allowed in our domain since it has 2 holes. If we want a sound approximation of that, we must remove at least one hole. We can then take E \ [1, 2] = a, E \ [3, 4] = b or even E (and of course, a lot of other things, with smaller holes than a or b). Whichever our abstract domain chooses, there is never both a /\ b < a and a /\ b < b. Indeed, this poset (like many real-world domains) is not a lattice, which doesn't keep us from speaking about soundness. An interesting and related fact is that there is no best abstraction of E \ [1, 2] \ [3, 4].

Maybe this digression about soundness rather hints that we should not compare speculative types during verification.

Finally, as a simple solution, one could simply run filter_speculative twice, that should be enough as the second filter will simply select the non empty speculative type if there is only one, and this one won't be above_centerline, or it would not exist as a speculative type already. To try to be a bit less aggressive, we can rather do that in case where we know it cannot be useful. If ft obtained from filter_speculative has no speculative type, but t has, we can know that it might be because it has been dropped, and computing t->filter_speculative(ft) could pick the speculative type of t. The speculative type can still be removed if the non-speculative type of ft is exact and non null for instance, but we've still reached a fixpoint, so it's correct, but a little bit too much work. This solution means that we are basically throwing away the speculative type in _type in case of clash. That sounds not shocking to me: if we get (hopefully) more accurate information on the inputs, it seems reasonable, or at least not shocking, to give up the previous speculative type if it clashes since it's not known to be correct. One can also phrase it as "we keep the speculative type of _type only if we don't have anything going against it".

This PR includes the last solution.

Thanks,
Marc

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8371716: C2: Phi node fails Value()'s verification when speculative types clash (Bug - P4)(⚠️ The fixVersion in this issue is [27] but the fixVersion in .jcheck/conf is 26, a new backport will be created when this pr is integrated.)

Contributors

• Roland Westrelin <roland@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28331/head:pull/28331
$ git checkout pull/28331

Update a local copy of the PR:
$ git checkout pull/28331
$ git pull https://git.openjdk.org/jdk.git pull/28331/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28331

View PR using the GUI difftool:
$ git pr show -t 28331

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28331.diff

Using Webrev

Link to Webrev Comment

[marc-chevalier]

/contributor add rwestrel

For the reproducer, now test.

[bridgekeeper]

👋 Welcome back mchevalier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@marc-chevalier rwestrel was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

• /contributor add @openjdk-bot
• /contributor add duke
• /contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

[openjdk]

@marc-chevalier The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[marc-chevalier]

/contributor add @rwestrel

[openjdk]

@marc-chevalier
Contributor Roland Westrelin <roland@openjdk.org> successfully added.

[mlbridge]

Webrevs

• 02: Full - Incremental (e9f3ac98)
• 01: Full - Incremental (7ac02cbf)
• 00: Full (818895ec)

[eme64]

@marc-chevalier I tried to review the issue at hand, but it is a little tricky because the regression test uses one set of class names and your description another (valhalla reproducer). Would it be possible to adjust the explanation to your mainline reproducer? Or maybe even just annotating your mainline reproducer with comments would help already.

I think it would be easier to review the PR once it is easy to follow the example.

Also: the issue name should be adjusted to say something a bit more specific about the issue. We have a lot of "missed optimization opportunity" issues, and it would be nice to be able to tell them apart easily ;)

[dafedafe]

More of a flyby rather than a review but I was wondering if it would make sense to extract this assert block since it is the same as the one above.

[marc-chevalier]

We surely can. But I'd rather avoid premature clean up as it is very uncertain to me how this issue will evolve, and maybe fundamentally change.

[marc-chevalier]

I've edited the description. I'd nevertheless suggest not to care too much about the test not to get sidetracked into details that are irrelevant to the issue. I actually regret I didn't write that symbolically.

[eme64]

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.

[eme64]

Suggested change

	// of the current `Value` call, `ft` (that is returned) is becoming `_type`. If verification happens then, `t` would be the
	// 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.

[eme64]

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? :)

[eme64]

And does cleanup_speculative happen during t->filter_speculative(_type), right?

[eme64]

I wonder if a minimal example would help here. I'm thinking something like this:

In rare cases, `_type` and `t` have incompatible opinion on speculative type, resulting into a too small intersection
t: Object (A)
_type: Object (B)
We filter them. Since A and B have no intersection, the speculative type is removed. This means the speculative type is implicitly "Object", and not TOP, as the intersection of A and B would suggest.
ft = t->filter_speculative(_type) = Object

After PhiNode::Value, we assign _type = ft. During verification, we run PhiNode::Value again, but this time:
t: Object (A) // same as above
_type: Object // ft from above
And now we get:
ft = t->filter_speculative(_type) = Object (A)

... argue about fixpoint next ...

It is a first proposal, and a little verbose... maybe you could find something more concise.

[marc-chevalier]

If we have no speculative type, it means we don't have a guess about what the type could be (more precisely than the actual type). You can say it's bottom, or that it's the same as the non-speculative type. And indeed, if you do t->filter_speculative(_type) when _type has no speculative type, the result has the same speculative type as t except when this one doesn't already pass cleanup_speculative, but then, it's still a fixpoint.

[marc-chevalier]

I don't see much value in your "minimal example". Isn't it just naming "A" and "B" instead of saying "speculative type of t" and "speculative type of _type"? I'm not fan of that: that just introduce more symbols, more names that are not in the code.

Also I'm very careful with examples, they are often misleading by giving a feeling that it's exactly how it happens. For instance Since A and B have no intersection is not always true, it just needs the intersection to be above centerline, a small enough intersection. I might rephrase later, but it seems you got what is happening and I first need a decision on the solution before spending time in cleanup that might be entirely erased. It seems that the fixpoint way was far from consensual.

[eme64]

Formatting looks a bit funny here.

[eme64]

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?

[eme64]

Or are you saying it is moved "from" rather than "into", i.e. this is some sort of code duplication?

[marc-chevalier]

This block is a duplication of the existing one, to go after the second filtering as @dafedafe noticed. I have no idea what is the comment for. As I told @dafedafe, if it stays, I'll factor it out, but it is premature cleanup at this point.

[eme64]

Suggested change

	if (jt->empty()) { // Emptied out???
	if (jt->empty()) {

Comment seems redundant.

[eme64]

Suggested change

	* -XX:+AlwaysIncrementalInline
	* -XX:+AlwaysIncrementalInline

GitHub Actions is giving us this:

Error: VM option 'AlwaysIncrementalInline' is develop and is available only in debug version of VM.
Improperly specified VM option 'AlwaysIncrementalInline'
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

Probably this would help: -XX:+IgnoreUnrecognizedVMOptions

[marc-chevalier]

I think reading the code and the comments to understand the situation might not be as good as reading the description of this PR. I regret I gave a reproducer and proposed a solution.

Given the very obvious lack of consensus on the Valhalla PR, it is clear that this issue might evolve radically and that the proposed solution may not be the final one. Therefore, I will not do any cleanup before agreeing on the way to go, as it might very well be erased and it would be a very poor use of everybody's time.

[eme64]

@marc-chevalier @merykitty @rwestrel I only just realized that the conversation from openjdk/valhalla#1717 still had relevance here. I had a very nice call with Marc, and he graciously explained things to me. I'll summarize my thoughts below.

It seems to be possible that the local speculative type of the phi (_type) and the speculative type of its inputs (t) do not have an intersection that is either null or top. Initially surprising: t->filter_speculative(_type) does not actually produce an intersection of the speculative type in this case, but removes the speculative type in cleanup_speculative because the type is above_centerline. Marc told me that Roland added this, to avoid "over speculation" because the type is now too narrow, and more likely to be wrong.

My question was if it is sane that we get this kind of inconsistent speculation: it must mean that our profiling has delivered somewhat inconsistent results. We would have to dig a bit deeper into the reproducer now, but it seems that an inlined (inner) method found one type during profiling, but the caller (outer method) found another at the phi. It would be interesting to understand a bit more what this implies. I could see these options:

• We assume both assumptions (speculations are correct). i.e. if there is a path for the type of the inner method to the place of the outer method, then it must be a subtype of what we profiled at both places. And it happens that this is only null or top, so we conclude it is impossible for something non-null to come through this path. It would be reasonable to speculate on this.
• Maybe profiling is incomplete: we see one type in the inner, and another type in the outer context. So it might be likely that both show up eventually at the phi.
• One more thought: the more speculative assumptions are "intersected", the more likely this combined speculation is to be incorrect at runtime. Conceptually, each speculation has a probability of failure, and if you apply many of them, the success probability goes lower and lower.

We could now dig deep into if Roland's cleanup_speculative logic that prevents "over speculation" is reasonable, or if we should extend it. But that goes out of scope for this bug fix here, in my opinion.

The question is now what we should do here. There are at least 3 options:

• The speculative type should be null, the natural intersection of the two speculative types. To me, that sounds like the most consistent solution. But it would require reconsidering the cleanup_speculative logic, a lot of effort. But maybe @merykitty has the desire to tackle such a project?
• The speculative type should be Object (or the union of the two speculative types). That is the same as removing the speculative type. But currently we don't mark the type as "widened", and so it is possible to later narrow it again once we filter again - so we were not able to reach a fixpoint yet, and that is a problem.
• Just pick one of the two speculative types. It's an arbitrary choice. But it's not incorrect. Marc's approach of filtering again acheives exactly that: first step removes the speculative type, the second step picks the speculative type of the incoming type. It seems to be the smallest code change, and so that may be best for a relative edge case like this.

There is a bit of a question how much of an edge case this really is. Probably quite. But we also cannot rely on our fuzzers here, as they don't really produce Object Classes at all.

Anyway: I'm the least qualified compared to @merykitty and @rwestrel , but I would vote for Marc's current proposal. Plus the required code cleanups, of course.

[merykitty]

I'm not sure if I'm correct, but I think speculative types themselves may not be consistent. For example, if they are consistent, then you will expect that the profiled types of the return values of a method a when calling from method b would be a subset of the profiled types of the returned values of a in general. However, this may not be the case, as we can ask for the second information first, then another type is introduced, then suddenly a method seems not to return a type C, but it does seem to return C if calling from b. As a result, maybe we can abandon trying to verify the correctness of speculative type computations.

Additionally, in the test case, the speculative type being empty is correct, the path is speculatively unreachable, maybe we can use that information to cut off the branches, simplify the CFG for better compilation?

[rwestrel]

Additionally, in the test case, the speculative type being empty is correct, the path is speculatively unreachable, maybe we can use that information to cut off the branches, simplify the CFG for better compilation?

Attached is another test case that reproduces the same issue.
TestSpeculativeTypes.java

I run that one with:

$ for i in seq 100; do java -XX:-TieredCompilation -XX:-UseOnStackReplacement -XX:-BackgroundCompilation -XX:CompileOnly=TestSpeculativeTypes::test2 -XX:CompileOnly=TestSpeculativeTypes::inlined3 -XX:CompileCommand=quiet -XX:TypeProfileLevel=200 -XX:+AlwaysIncrementalInline -XX:VerifyIterativeGVN=10 -XX:CompileCommand=dontinline,TestSpeculativeTypes::notInlined1 -XX:+StressIncrementalInlining TestSpeculativeTypes || break; done

It usually fails after a few runs. That one has conflicting profile data but no dead path. What you're suggesting has some risk and unclear benefits so I think would need to be investigated separately.

[eme64]

@rwestrel @merykitty @marc-chevalier Suggestion:

• Marc adds Roland's new reproducer.
• Marc annotates both the existing and Roland/s new reproducer: show where the speculative types come from, and where they flow, and where the Phi with the conflicting speculative type is located.
• We keep the speculative type verification. Because I'm not convinced that we should just remove verification yet.

In future RFE's we could consider:

• Removing verification for speculative types. But I wonder if that is smart.
• Reconsider if we should really do the "widening" if we have above_centerline. As Roland said: there are risks to this. There must have been some historical reason for this. But who knows, maybe it hurts in more cases than it helps. Speculation is always based on a heuristic, and we would need a few benchmarks to show the impact, and we would also need to run some bigger benchmarks to see the impact. That is a lot of effort that would need to happen outside this bugfix. I suppose it is a tradeoff between code size (win if we cut paths) and cost of recompilation (if we fail speculative checks)?

---

I suppose it depends on what our definition of "consistent" is for speculative types. Of course they can be wrong at runtime, that is the whole point of speculation, so in a sense that makes them "inconsistent". But we could at least enforce some "consistency" in the computation that we do with the speculative types: intersections and unions of types should be done "consistently", I suppose.

I'm trying to reconstruct Quan-Anh's definition of "consistent":

For example, if they are consistent, then you will expect that the profiled types of the return values of a method a when calling from method b would be a subset of the profiled types of the returned values of a in general.

Does this kind of problem not also arise for non-speculative types, at least during CCP or IGVN? First I'm thinking of Casts, but maybe those are special. They can have narrower type than their input type. What about Phi nodes? During IGVN, do we always expect the input types to be a subtype of the output type? I guess so?

Observation: "widening" like that what happens here because of above_centerline and elsewhere like integer types with their "widening" (e.g. widening because loop phi continually grows the range) still means that input types are a subset of the output type.

So according to Quan-Anh's definition, we start with an inconsistent speculative type. But would it not be nice if it eventually became consistent, specifically after CCP/IGVN. And we could already get local consistency after every Value call, right? If yes: it would be nice to have verification for that.

[marc-chevalier]

In future RFE's we could consider:

I might have missed something, but which solution would you then include for now?

[eme64]

In future RFE's we could consider:

I might have missed something, but which solution would you then include for now?

The one you have now: one more round of filtering to get to fixpoint ;)

[chhagedorn]

But would it not be nice if it eventually became consistent, specifically after CCP/IGVN.

Just a side node: We remove the speculative types after incremental inlining here:

jdk/src/hotspot/share/opto/compile.cpp

Lines 2359 to 2363 in cc5b35b

	// Remove the speculative part of types and clean up the graph from
	// the extra CastPP nodes whose only purpose is to carry them. Do
	// that early so that optimizations are not disrupted by the extra
	// CastPP nodes.
	remove_speculative_types(igvn);

[marc-chevalier]

Interesting, and indeed, in what I know, we crash inside the inline_incrementally, a few lines above.