diff --git a/src/hotspot/share/opto/loopnode.cpp b/src/hotspot/share/opto/loopnode.cpp index 1018a8ae9ab..e7ccefa6855 100644 --- a/src/hotspot/share/opto/loopnode.cpp +++ b/src/hotspot/share/opto/loopnode.cpp @@ -3856,6 +3856,7 @@ const TypeInt* CountedLoopConverter::filtered_type(Node* n, Node* n_ctrl) { Node* region = phi->in(0); assert(n_ctrl == nullptr || n_ctrl == region, "ctrl parameter must be region"); if (region && region != _phase->C->top()) { + // Compute the union over the types of the paths/inputs. for (uint i = 1; i < phi->req(); i++) { Node* val = phi->in(i); Node* use_c = region->in(i); @@ -3866,10 +3867,17 @@ const TypeInt* CountedLoopConverter::filtered_type(Node* n, Node* n_ctrl) { } else { filtered_t = filtered_t->meet(val_t)->is_int(); } + } else { + // We found no constriant, so we have to assume that this path + // is unconstrained, i.e. it could have the whole int range. + filtered_t = TypeInt::INT; } } } } + + // The filtered type may be worse than what we already know + // about n, so take the intersection. const TypeInt* n_t = _phase->igvn().type(n)->is_int(); if (filtered_t != nullptr) { n_t = n_t->join(filtered_t)->is_int(); @@ -3880,6 +3888,8 @@ const TypeInt* CountedLoopConverter::filtered_type(Node* n, Node* n_ctrl) { //------------------------------filtered_type_from_dominators-------------------------------- // Return a possibly more restrictive type for val based on condition control flow of dominators +// Note: we can also return "nullptr", which means "no constraint", and should be interpreted +// as if we returned TypeInt::INT. const TypeInt* CountedLoopConverter::filtered_type_from_dominators(Node* val, Node* use_ctrl) { if (val->is_Con()) { return val->bottom_type()->is_int(); diff --git a/test/hotspot/jtreg/compiler/loopopts/TestHasTruncationWrap.java b/test/hotspot/jtreg/compiler/loopopts/TestHasTruncationWrap.java index efc3831dc3a..9a68a2fcb77 100644 --- a/test/hotspot/jtreg/compiler/loopopts/TestHasTruncationWrap.java +++ b/test/hotspot/jtreg/compiler/loopopts/TestHasTruncationWrap.java @@ -353,6 +353,24 @@ public class TestHasTruncationWrap { // testIRShort2b: short loop, ranges proved in short range via CmpI before loop. // Compared to testIRShort2, the check in the loop is an NEQ. + // + // Since the bug fix of JDK-8386830, we no longer allow this case to detect CountedLoop: + // The backedge finds no useful constraint, the "i != limit" does not give any restrictions, + // and so we have to assume it produces the full range. + // Comparing with testIRShort2, there we have a useful check "i < limit", which does + // give us a restriction, that helps us prove there is not wrap overflow. + // + // In the future, we could try to do something more smart, and combine the info about + // entry type "init < limit <= 100" with the fact that we have unity-stride, and so + // we should not be able to skip the NEQ "i != limit", and be able to canonicalize + // NEQ to LT. But for now, I consider this an edge-case that we will just have to accept + // will not be optimized to CountedLoop for now. For now, a workaround is using the + // exit condition "i < limit". + // This is really a problem about iv evolution (iv starts in range, increments by 1, + // and cannot skip exit check, so NEQ can be converted to LT), and cannot be solved + // by the type info of entry/backedge separately, so I don't have a quick fix here. + // We do this NEQ to LT canonicalization for int loops, but we would also need + // dedicated logic for it specifically combined with the wrap-detection logic. public static int testIRShort2b_gold = testIRShort2b(); @Run(test = "testIRShort2b") @@ -362,7 +380,7 @@ public class TestHasTruncationWrap { } @Test - @IR(counts = {IRNode.COUNTED_LOOP, "> 0"}) + @IR(counts = {IRNode.COUNTED_LOOP, "= 0"}) static int testIRShort2b() { int init = Math.max(lo, 0); // init in [0..max_int] int limit = Math.min(hi, 100); // limit in [min_int..100] @@ -374,10 +392,8 @@ public class TestHasTruncationWrap { int sum = 0; for (int i = init; i != limit; i = (short)(i+1)) { sum = opaqueSum(sum); // work to keep loop alive - // The backedge value of i is also far - // enough from short boundaries, because of - // the loop exit check: - // i < limit <= 100 + // Unfortunately, the backedge does not produce a useful + // check with "i != limit", and so the type is unconstrained. } return sum; } @@ -589,8 +605,15 @@ public class TestHasTruncationWrap { } // testIRShort5d: short while-loop, again similar to testIRShort2b and testIRShort5c, but with while-loop form. - // No peeling, and so the entry value is init, and so the "init >= limit" check is useful, - // and used by has_truncation_wrap. With it, C2 manages to prove no short-overflow. + // + // Same issue as with testIRShort2b: + // After JDK-8386830, we now see that the backedge type is not constrained, + // and so don't allow CountedLoop detection. + // However, we could be smarter in the future, and canonicalize NEQ + // to LT, because this is a unity-stride loop where the "i != limit" + // can provably not be skipped. For now, we just have to accept that + // we cannot optimize this, and people would have to use "i < limit", + // see testIRShort5. public static int testIRShort5d_gold = testIRShort5d(); @Run(test = "testIRShort5d") @@ -600,7 +623,7 @@ public class TestHasTruncationWrap { } @Test - @IR(counts = {IRNode.COUNTED_LOOP, "> 0"}) + @IR(counts = {IRNode.COUNTED_LOOP, "= 0"}) static int testIRShort5d() { int init = Math.max(lo, 0); // init in [0..max_int] int limit = Math.min(hi, 100); // limit in [min_int..100] @@ -614,6 +637,8 @@ public class TestHasTruncationWrap { while (i != limit) { sum = opaqueSum(sum); // work to keep loop alive i = (short)(i+1); + // Unfortunately, the backedge does not produce a useful + // check with "i != limit", and so the type is unconstrained. } return sum; } diff --git a/test/hotspot/jtreg/compiler/loopopts/TestTruncationWrapPhiTypeUnion.java b/test/hotspot/jtreg/compiler/loopopts/TestTruncationWrapPhiTypeUnion.java new file mode 100644 index 00000000000..a7873359b45 --- /dev/null +++ b/test/hotspot/jtreg/compiler/loopopts/TestTruncationWrapPhiTypeUnion.java @@ -0,0 +1,221 @@ +/* + * Copyright (c) 2026, 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. + */ + +package compiler.loopopts; + +/* + * @test + * @bug 8386830 + * @summary Test for CountedLoopConverter::filtered_type, where we wrongly + * ignored a nullptr type, and returned a type that was too narrow, + * which led us to wrongly ignore wrapping in + * CountedLoopConverter::has_truncation_wrap + * @library /test/lib / + * @run main/othervm -Xbatch + * -XX:CompileCommand=compileonly,${test.main.class}::test* + * ${test.main.class} + * @run main ${test.main.class} + */ +public class TestTruncationWrapPhiTypeUnion { + + interface TestMethod { + int call(); + } + + public static void main(String[] args) { + int failures = 0; + + failures += run("test1", () -> test1(-1), 11111); + failures += run("test2", () -> test2(-1), 11111); + failures += run("test3", () -> test3(-100_000), -87065049); + failures += run("test4", () -> test4(32770), 10330); + failures += run("test5", () -> test5(-63), 10340); + + if (failures > 0) { + throw new RuntimeException("failures: " + failures); + } + } + + static int run(String name, TestMethod t, int expected) { + for (int i = 0; i < 20; i++) { + int result = t.call(); + if (result != expected) { + System.out.println(name + " wrong result: " + result + " vs " + expected); + return 1; + } + } + return 0; + } + + static int test1(int limit) { + int x = 0; + int sum = 0; + + limit = (byte) limit; // type BYTE = [-128..127], at runtime: -1 + + int i = 510; + while (limit < i) { + // Exit check checks for positive values, but with + // entry 510 and unsigned truncation, that can never fail. + + sum++; + + // Secondary exit check, to make sure we exit eventually. + if (++x >= 11111) { + break; + } + + // Unsigned 15-bit truncation. + // We check for wrap/truncation/underflow: + // + // } else if (stride_con < 0) { + // if (truncation.trunc_type()->lo_as_long() - phi_ft->lo_as_long() > stride_con || + // truncation.trunc_type()->hi_as_long() < phi_ft->hi_as_long()) { + // return true; // truncation may occur + // } + // } + // + // The lo of truncation is 0, and also the phi type should have a lo of 0, + // but it was wrongly determined to be 510. + // So, whereas "0 - 0 > -10" would have given us the required "true", + // we now checked "0 - 510 > -10", which was wrongly "false". + // + // The reason is that the phi_ft has been wrongly determined to be 510, + // so only considering the entry value. This is determined inside filtered_type: + // - entry: filtered_type_from_dominators discovers entry value 510. + // - backedge: filtered_type_from_dominators discovers no dominating-if, returns nullptr. + // But filtered_type skips nullptr results, as in "no extra filter". But + // we should be accumulating the entry and backedge type here! + // + // The only if on the backedge-path would have been the exit + // check: limit < i. But filtered_int_type finds nothing, returns nullptr. + i = (i - 10) & 0x7fff; + } + + return sum; + } + + // Note: this case was first discovered during JDK-8386591, which enabled 0xffff masking. + // Without allowing 0xffff masking, this did not fail. But it was the way I + // first discovered the bug, and so I wanted to add this as a test anyway. + static int test2(int limit) { + int x = 0; + int sum = 0; + + limit = (short) limit; // type SHORT = [-32768..32767], at runtime: -1 + + int i = 510; + while (limit < i) { + // Exit check checks for positive values, but with + // entry 510 and unsigned truncation, that can never fial. + + sum++; + + // Secondary exit check, to make sure we exit eventually. + if (++x >= 11111) { + break; + } + + // CHAR truncation: 0..0xffff = 0..65535 + // + // i iterates: 510, 500, ... 10, 0 + // And then, it shshould ould underflows: (0 - 10) & 0xffff = 65526 + // + // But in CountedLoopConverter::has_truncation_wrap, we wrongly + // decide there cannot be overflow. + // truncation: [0..65535] + // stride_con: -10 + // + // Accordingly, phi_ft should be in [0..65535], and so when we check + // for underflow, we check: + // + // } else if (stride_con < 0) { + // if (truncation.trunc_type()->lo_as_long() - phi_ft->lo_as_long() > stride_con || + // truncation.trunc_type()->hi_as_long() < phi_ft->hi_as_long()) { + // return true; // truncation may occur + // } + // } + // + // So we should check: 0 - 0 > -10, and we would see that truncation could occur. + // But instead, we checked 0 - 510 > -10, which wronly lead to "no truncation". + // + // The reason is that the phi_ft has been wrongly determined to be 510, + // so only considering the entry value. This is determined inside filtered_type: + // - entry: filtered_type_from_dominators discovers entry value 510. + // - backedge: filtered_type_from_dominators discovers no dominating-if, returns nullptr. + // But filtered_type skips nullptr results, as in "no extra filter". But + // we should be accumulating the entry and backedge type here! + // + // The only if on the backedge-path would have been the exit + // check: limit < i. But filtered_int_type finds nothing, returns nullptr. + i = (i - 10) & 0xffff; + } + + return sum; + } + + // Another fuzzer find, this one with short truncation. + static int test3(int limit) { + int x = 0; + int sum = 0; + + // Range: [min_int..8192], at runtime: -100_000 + limit = Math.min(limit, 8192); + int i; + for (i = 128; limit <= i; i = (short)(i - 16384)) { + sum = sum + i + 1; + if (x++ > 10789) { + break; + } + } + return sum + i; + } + + // Another fuzzer find, this one with short truncation. + static int test4(int limit) { + int sum = 0; + int x = 0; + limit = (short) limit; + for (int i = -1025; limit <= i; i = (short) (i + -7)) { + sum = sum + 1; + if (x++ > 10328) { + break; + } + } + return sum; + } + + // Another fuzzer find, again with 0x7fff mask. + static int test5(int limit) { + int x = 0; + int sum = 0; + limit = (byte)limit; + for (int i = 8192; limit < i; i = ((i + -8) & 0x7fff)) { + sum++; + if (x++ > 10338) { + break; + } + } + return sum; + } +}