infernap12/jdk
1
0
Fork 0
mirror of https://github.com/openjdk/jdk.git synced 2026-07-02 15:20:27 +00:00
Code Issues Packages Projects Releases Wiki Activity

8386830: C2: CountedLoopConverter::filtered_type wrongly ignores nullptr contributions to type union/meet

Browse Source 
Reviewed-by: qamai, kvn
This commit is contained in:
Emanuel Peter 2026-06-19 10:58:31 +00:00
parent c2f0259304
commit 2f37461c25
3 changed files with 264 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/hotspot/share/opto/loopnode.cpp
View File

@ -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();

41
test/hotspot/jtreg/compiler/loopopts/TestHasTruncationWrap.java
View File

@ -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;
}

221
test/hotspot/jtreg/compiler/loopopts/TestTruncationWrapPhiTypeUnion.java Normal file
View File

@ -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;
}
}