Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions src/hotspot/share/opto/cfgnode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1351,6 +1351,82 @@ const Type* PhiNode::Value(PhaseGVN* phase) const {
}
#endif //ASSERT


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// same (union of input types), but the new `_type` has now no speculative type, the result of `t->filter_speculative(_type)`
// 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
// (like the previously returned type). In such a case, doing the filtering one time more allows to reach a fixpoint.
Comment on lines +1354 to +1360
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@marc-chevalier marc-chevalier Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (ft->speculative() == nullptr && t->speculative() != nullptr) {
const Type* first_ft = ft;
ft = t->filter_speculative(first_ft);
#ifdef ASSERT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// The following logic has been moved into TypeOopPtr::filter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const Type* jt = t->join_speculative(first_ft);
if (jt->empty()) { // Emptied out???
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment seems redundant.

// Otherwise it's something stupid like non-overlapping int ranges
// found on dying counted loops.
assert(ft == Type::TOP, ""); // Canonical empty value
}

else {

if (jt != ft && jt->base() == ft->base()) {
Comment on lines +1370 to +1375
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting looks a bit funny here.

if (jt->isa_int() &&
jt->is_int()->_lo == ft->is_int()->_lo &&
jt->is_int()->_hi == ft->is_int()->_hi)
jt = ft;
if (jt->isa_long() &&
jt->is_long()->_lo == ft->is_long()->_lo &&
jt->is_long()->_hi == ft->is_long()->_hi)
jt = ft;
}
if (jt != ft) {
tty->print("merge type: "); t->dump(); tty->cr();
tty->print("kill type: "); _type->dump(); tty->cr();
tty->print("join type: "); jt->dump(); tty->cr();
tty->print("filter type: "); ft->dump(); tty->cr();
}
assert(jt == ft, "");
}
#endif //ASSERT
}

#ifdef ASSERT
const Type* ft_ = t->filter_speculative(ft);
if (!Type::equals(ft, ft_)) {
stringStream ss;

for (uint i = 1; i < req(); ++i) {
ss.print("in(%d): ", i);
if (r->in(i) && phase->type(r->in(i)) == Type::CONTROL) {
const Type* ti = phase->type(in(i));
ti->dump_on(&ss);
}
ss.print_cr("");
}

ss.print("t: ");
t->dump_on(&ss);
ss.print_cr("");

ss.print("_type: ");
_type->dump_on(&ss);
ss.print_cr("");

ss.print("Filter once: ");
ft->dump_on(&ss);
ss.print_cr("");
ss.print("Filter twice: ");
ft_->dump_on(&ss);
ss.print_cr("");
tty->print("%s", ss.base());
tty->flush();
assert(false, "computed type would not pass verification");
}
#endif

// Deal with conversion problems found in data loops.
ft = phase->saturate_and_maybe_push_to_igvn_worklist(this, ft);
return ft;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Copyright (c) 2025, Red Hat, Inc.
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/**
* @test
* @bug 8360561
* @summary Ranges can be proven to be disjoint but not orderable (thanks to unsigned range)
* Comparing such values in such range with != should always be true.
* @run main/othervm -XX:-TieredCompilation
* -XX:-UseOnStackReplacement
* -XX:-BackgroundCompilation
* -XX:CompileOnly=compiler.igvn.ClashingSpeculativeTypePhiNode::test1
* -XX:CompileCommand=quiet
* -XX:TypeProfileLevel=222
* -XX:+AlwaysIncrementalInline
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* -XX:VerifyIterativeGVN=10
* -XX:CompileCommand=dontinline,compiler.igvn.ClashingSpeculativeTypePhiNode::notInlined1
* compiler.igvn.ClashingSpeculativeTypePhiNode
*
* @run main compiler.igvn.ClashingSpeculativeTypePhiNode
*/

package compiler.igvn;

public class ClashingSpeculativeTypePhiNode {
public static void main(String[] args) {
for (int i = 0; i < 20_000; i++) {
test1(false);
inlined1(true, true);
inlined2(false);
}
}

private static Object test1(boolean flag1) {
return inlined1(flag1, false);
}

private static Object inlined1(boolean flag1, boolean flag2) {
if (flag1) {
return inlined2(flag2); // C1
}
return null;
}

private static Object inlined2(boolean flag2) {
if (flag2) {
return new C1();
}
return notInlined1(); // C2
}

private static Object notInlined1() {
return new C2();
}

static class C1 {

}

static class C2 {

}
}