-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8371716: C2: Phi node fails Value()'s verification when speculative types clash #28331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||
| // 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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:
Can you clarify please for my understanding? :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And does
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: It is a first proposal, and a little verbose... maybe you could find something more concise.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Also I'm very careful with examples, they are often misleading by giving a feeling that it's exactly how it happens. For instance |
||||||
| if (ft->speculative() == nullptr && t->speculative() != nullptr) { | ||||||
| const Type* first_ft = ft; | ||||||
| ft = t->filter_speculative(first_ft); | ||||||
| #ifdef ASSERT | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| const Type* jt = t->join_speculative(first_ft); | ||||||
| if (jt->empty()) { // Emptied out??? | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||
|
|
||||||
| 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 | ||||||
marc-chevalier marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| * @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 | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
GitHub Actions is giving us this: Probably this would help: |
||||||
| * -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 { | ||||||
|
|
||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.