stan/jdk-24
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

8307683: Loop Predication should not hoist range checks with trap on success projection by negating their condition

Browse Source 
Reviewed-by: thartmann, roland
This commit is contained in:
Christian Hagedorn 2023-06-01 08:04:45 +00:00
parent 96ed1392d1
commit dfd3da3f52
5 changed files with 252 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
src/hotspot/share/opto/loopPredicate.cpp
View File

@ -831,8 +831,9 @@ class Invariance : public StackObj {
// Returns true if the predicate of iff is in "scale*iv + offset u< load_range(ptr)" format
// Note: this function is particularly designed for loop predication. We require load_range
// and offset to be loop invariant computed on the fly by "invar"
bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, BasicType bt, Node *iv, Node *&range,
bool IdealLoopTree::is_range_check_if(IfProjNode* if_success_proj, PhaseIdealLoop *phase, BasicType bt, Node *iv, Node *&range,
Node *&offset, jlong &scale) const {
IfNode* iff = if_success_proj->in(0)->as_If();
if (!is_loop_exit(iff)) {
return false;
}
@ -840,7 +841,43 @@ bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, BasicT
return false;
}
const BoolNode *bol = iff->in(1)->as_Bool();
if (bol->_test._test != BoolTest::lt) {
if (bol->_test._test != BoolTest::lt || if_success_proj->is_IfFalse()) {
// We don't have the required range check pattern:
// if (scale*iv + offset <u limit) {
//
// } else {
// trap();
// }
//
// Having the trap on the true projection:
// if (scale*iv + offset <u limit) {
// trap();
// }
//
// is not correct. We would need to flip the test to get the expected "trap on false path" pattern:
// if (scale*iv + offset >=u limit) {
//
// } else {
// trap();
// }
//
// If we create a Hoisted Range Check Predicate for this wrong pattern, it could succeed at runtime (i.e. true
// for the value of "scale*iv + offset" in the first loop iteration and true for the value of "scale*iv + offset"
// in the last loop iteration) while the check to be hoisted could fail in other loop iterations.
//
// Example:
// Loop: "for (int i = -1; i < 1000; i++)"
// init = "scale*iv + offset" in the first loop iteration = 1*-1 + 0 = -1
// last = "scale*iv + offset" in the last loop iteration = 1*999 + 0 = 999
// limit = 100
//
// Hoisted Range Check Predicate is always true:
// init >=u limit && last >=u limit <=>
// -1 >=u 100 && 999 >= u 100
//
// But for 0 <= x < 100: x >=u 100 is false.
// We would wrongly skip the branch with the trap() and possibly miss to execute some other statements inside that
// trap() branch.
return false;
}
if (!bol->in(1)->is_Cmp()) {
@ -871,14 +908,14 @@ bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, BasicT
return true;
}
bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invariance& invar DEBUG_ONLY(COMMA ProjNode *predicate_proj)) const {
bool IdealLoopTree::is_range_check_if(IfProjNode* if_success_proj, PhaseIdealLoop *phase, Invariance& invar DEBUG_ONLY(COMMA ProjNode *predicate_proj)) const {
Node* range = nullptr;
Node* offset = nullptr;
jlong scale = 0;
Node* iv = _head->as_BaseCountedLoop()->phi();
Compile* C = Compile::current();
const uint old_unique_idx = C->unique();
if (!is_range_check_if(iff, phase, T_INT, iv, range, offset, scale)) {
if (!is_range_check_if(if_success_proj, phase, T_INT, iv, range, offset, scale)) {
return false;
}
if (!invar.is_invariant(range)) {
@ -931,10 +968,8 @@ bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invari
// max(scale*i + offset) = scale*(limit-stride) + offset
// (2) stride*scale < 0
// max(scale*i + offset) = scale*init + offset
BoolNode* PhaseIdealLoop::rc_predicate(IdealLoopTree *loop, Node* ctrl,
int scale, Node* offset,
Node* init, Node* limit, jint stride,
Node* range, bool upper, bool &overflow, bool negate) {
BoolNode* PhaseIdealLoop::rc_predicate(IdealLoopTree* loop, Node* ctrl, int scale, Node* offset, Node* init,
Node* limit, jint stride, Node* range, bool upper, bool& overflow) {
jint con_limit = (limit != nullptr && limit->is_Con()) ? limit->get_int() : 0;
jint con_init = init->is_Con() ? init->get_int() : 0;
jint con_offset = offset->is_Con() ? offset->get_int() : 0;
@ -1060,7 +1095,7 @@ BoolNode* PhaseIdealLoop::rc_predicate(IdealLoopTree *loop, Node* ctrl,
cmp = new CmpUNode(max_idx_expr, range);
}
register_new_node(cmp, ctrl);
BoolNode* bol = new BoolNode(cmp, negate ? BoolTest::ge : BoolTest::lt);
BoolNode* bol = new BoolNode(cmp, BoolTest::lt);
register_new_node(bol, ctrl);
if (TraceLoopPredicate) {
@ -1323,12 +1358,12 @@ void PhaseIdealLoop::loop_predication_follow_branches(Node *n, IdealLoopTree *lo
} while (stack.size() > 0);
}
bool PhaseIdealLoop::loop_predication_impl_helper(IdealLoopTree* loop, IfProjNode* if_proj,
bool PhaseIdealLoop::loop_predication_impl_helper(IdealLoopTree* loop, IfProjNode* if_success_proj,
ParsePredicateSuccessProj* parse_predicate_proj, CountedLoopNode* cl,
ConNode* zero, Invariance& invar, Deoptimization::DeoptReason reason) {
// Following are changed to nonnull when a predicate can be hoisted
IfProjNode* new_predicate_proj = nullptr;
IfNode* iff = if_proj->in(0)->as_If();
IfNode* iff = if_success_proj->in(0)->as_If();
Node* test = iff->in(1);
if (!test->is_Bool()) { //Conv2B, ...
return false;
@ -1344,7 +1379,7 @@ bool PhaseIdealLoop::loop_predication_impl_helper(IdealLoopTree* loop, IfProjNod
// Negate test if necessary (Parse Predicates always have IfTrue as success projection and IfFalse as uncommon trap)
bool negated = false;
if (if_proj->is_IfFalse()) {
if (if_success_proj->is_IfFalse()) {
new_predicate_bol = new BoolNode(new_predicate_bol->in(1), new_predicate_bol->_test.negate());
register_new_node(new_predicate_bol, ctrl);
negated = true;
@ -1361,8 +1396,9 @@ bool PhaseIdealLoop::loop_predication_impl_helper(IdealLoopTree* loop, IfProjNod
loop->dump_head();
}
#endif
} else if (cl != nullptr && loop->is_range_check_if(iff, this, invar DEBUG_ONLY(COMMA parse_predicate_proj))) {
} else if (cl != nullptr && loop->is_range_check_if(if_success_proj, this, invar DEBUG_ONLY(COMMA parse_predicate_proj))) {
// Range check for counted loops
assert(if_success_proj->is_IfTrue(), "trap must be on false projection for a range check");
const Node* cmp = bol->in(1)->as_Cmp();
Node* idx = cmp->in(1);
assert(!invar.is_invariant(idx), "index is variant");
@ -1397,33 +1433,31 @@ bool PhaseIdealLoop::loop_predication_impl_helper(IdealLoopTree* loop, IfProjNod
}
// If predicate expressions may overflow in the integer range, longs are used.
bool overflow = false;
// Negate test if necessary (Parse Predicates always have IfTrue as success projection and IfFalse as uncommon trap)
const bool negate = (if_proj->is_IfFalse());
// Test the lower bound
BoolNode* lower_bound_bol = rc_predicate(loop, ctrl, scale, offset, init, limit, stride, rng, false, overflow, negate);
BoolNode* lower_bound_bol = rc_predicate(loop, ctrl, scale, offset, init, limit, stride, rng, false, overflow);
const int if_opcode = iff->Opcode();
IfProjNode* lower_bound_proj = create_new_if_for_predicate(parse_predicate_proj, nullptr, reason, overflow ? Op_If : if_opcode);
IfNode* lower_bound_iff = lower_bound_proj->in(0)->as_If();
_igvn.hash_delete(lower_bound_iff);
lower_bound_iff->set_req(1, lower_bound_bol);
if (TraceLoopPredicate) tty->print_cr("lower bound check if: %s %d ", negate ? " negated" : "", lower_bound_iff->_idx);
if (TraceLoopPredicate) tty->print_cr("lower bound check if: %d", lower_bound_iff->_idx);
// Test the upper bound
BoolNode* upper_bound_bol = rc_predicate(loop, lower_bound_proj, scale, offset, init, limit, stride, rng, true, overflow, negate);
BoolNode* upper_bound_bol = rc_predicate(loop, lower_bound_proj, scale, offset, init, limit, stride, rng, true,
overflow);
IfProjNode* upper_bound_proj = create_new_if_for_predicate(parse_predicate_proj, nullptr, reason, overflow ? Op_If : if_opcode);
assert(upper_bound_proj->in(0)->as_If()->in(0) == lower_bound_proj, "should dominate");
IfNode* upper_bound_iff = upper_bound_proj->in(0)->as_If();
_igvn.hash_delete(upper_bound_iff);
upper_bound_iff->set_req(1, upper_bound_bol);
if (TraceLoopPredicate) tty->print_cr("upper bound check if: %s %d ", negate ? " negated" : "", lower_bound_iff->_idx);
if (TraceLoopPredicate) tty->print_cr("upper bound check if: %d", lower_bound_iff->_idx);
// Fall through into rest of the cleanup code which will move any dependent nodes to the skeleton predicates of the
// upper bound test. We always need to create skeleton predicates in order to properly remove dead loops when later
// splitting the predicated loop into (unreachable) sub-loops (i.e. done by unrolling, peeling, pre/main/post etc.).
new_predicate_proj = add_template_assertion_predicate(iff, loop, if_proj, parse_predicate_proj, upper_bound_proj, scale,
new_predicate_proj = add_template_assertion_predicate(iff, loop, if_success_proj, parse_predicate_proj, upper_bound_proj, scale,
offset, init, limit, stride, rng, overflow, reason);
#ifndef PRODUCT
@ -1439,10 +1473,10 @@ bool PhaseIdealLoop::loop_predication_impl_helper(IdealLoopTree* loop, IfProjNod
}
assert(new_predicate_proj != nullptr, "sanity");
// Success - attach condition (new_predicate_bol) to predicate if
invar.map_ctrl(if_proj, new_predicate_proj); // so that invariance test can be appropriate
invar.map_ctrl(if_success_proj, new_predicate_proj); // so that invariance test can be appropriate
// Eliminate the old If in the loop body
dominated_by(new_predicate_proj, iff, if_proj->_con != new_predicate_proj->_con );
dominated_by(new_predicate_proj, iff, if_success_proj->_con != new_predicate_proj->_con);
C->set_major_progress();
return true;
@ -1459,7 +1493,8 @@ IfProjNode* PhaseIdealLoop::add_template_assertion_predicate(IfNode* iff, IdealL
Node* opaque_init = new OpaqueLoopInitNode(C, init);
register_new_node(opaque_init, upper_bound_proj);
bool negate = (if_proj->_con != predicate_proj->_con);
BoolNode* bol = rc_predicate(loop, upper_bound_proj, scale, offset, opaque_init, limit, stride, rng, (stride > 0) != (scale > 0), overflow, negate);
BoolNode* bol = rc_predicate(loop, upper_bound_proj, scale, offset, opaque_init, limit, stride, rng,
(stride > 0) != (scale > 0), overflow);
Node* opaque_bol = new Opaque4Node(C, bol, _igvn.intcon(1)); // This will go away once loop opts are over
C->add_template_assertion_predicate_opaq(opaque_bol);
register_new_node(opaque_bol, upper_bound_proj);
@ -1481,7 +1516,8 @@ IfProjNode* PhaseIdealLoop::add_template_assertion_predicate(IfNode* iff, IdealL
max_value = new CastIINode(max_value, loop->_head->as_CountedLoop()->phi()->bottom_type());
register_new_node(max_value, predicate_proj);
bol = rc_predicate(loop, new_proj, scale, offset, max_value, limit, stride, rng, (stride > 0) != (scale > 0), overflow, negate);
bol = rc_predicate(loop, new_proj, scale, offset, max_value, limit, stride, rng, (stride > 0) != (scale > 0),
overflow);
opaque_bol = new Opaque4Node(C, bol, _igvn.intcon(1));
C->add_template_assertion_predicate_opaq(opaque_bol);
register_new_node(opaque_bol, new_proj);

2
src/hotspot/share/opto/loopTransform.cpp
View File

@ -2858,7 +2858,7 @@ Node* PhaseIdealLoop::add_range_check_elimination_assertion_predicate(IdealLoopT
Node* value) {
bool overflow = false;
BoolNode* bol = rc_predicate(loop, ctrl, scale_con, offset, value, nullptr, stride_con,
limit, (stride_con > 0) != (scale_con > 0), overflow, false);
limit, (stride_con > 0) != (scale_con > 0), overflow);
Node* opaque_bol = new Opaque4Node(C, bol, _igvn.intcon(1));
register_new_node(opaque_bol, ctrl);
IfNode* new_iff = nullptr;

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

@ -1089,13 +1089,13 @@ int PhaseIdealLoop::extract_long_range_checks(const IdealLoopTree* loop, jlong s
for (uint i = 0; i < loop->_body.size(); i++) {
Node* c = loop->_body.at(i);
if (c->is_IfProj() && c->in(0)->is_RangeCheck()) {
CallStaticJavaNode* call = c->as_IfProj()->is_uncommon_trap_if_pattern(Deoptimization::Reason_none);
IfProjNode* if_proj = c->as_IfProj();
CallStaticJavaNode* call = if_proj->is_uncommon_trap_if_pattern(Deoptimization::Reason_none);
if (call != nullptr) {
Node* range = nullptr;
Node* offset = nullptr;
jlong scale = 0;
RangeCheckNode* rc = c->in(0)->as_RangeCheck();
if (loop->is_range_check_if(rc, this, T_LONG, phi, range, offset, scale) &&
if (loop->is_range_check_if(if_proj, this, T_LONG, phi, range, offset, scale) &&
loop->is_invariant(range) && loop->is_invariant(offset) &&
original_iters_limit / ABS(scale * stride_con) >= min_iters) {
reduced_iters_limit = MIN2(reduced_iters_limit, original_iters_limit/ABS(scale));

13
src/hotspot/share/opto/loopnode.hpp
View File

@ -733,8 +733,8 @@ public:
bool policy_range_check(PhaseIdealLoop* phase, bool provisional, BasicType bt) const;
// Return TRUE if "iff" is a range check.
bool is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invariance& invar DEBUG_ONLY(COMMA ProjNode *predicate_proj)) const;
bool is_range_check_if(IfNode* iff, PhaseIdealLoop* phase, BasicType bt, Node* iv, Node*& range, Node*& offset,
bool is_range_check_if(IfProjNode* if_success_proj, PhaseIdealLoop* phase, Invariance& invar DEBUG_ONLY(COMMA ProjNode* predicate_proj)) const;
bool is_range_check_if(IfProjNode* if_success_proj, PhaseIdealLoop* phase, BasicType bt, Node* iv, Node*& range, Node*& offset,
jlong& scale) const;
// Estimate the number of nodes required when cloning a loop (body).
@ -1366,15 +1366,12 @@ public:
void register_control(Node* n, IdealLoopTree *loop, Node* pred, bool update_body = true);
// Construct a range check for a predicate if
BoolNode* rc_predicate(IdealLoopTree *loop, Node* ctrl,
int scale, Node* offset,
Node* init, Node* limit, jint stride,
Node* range, bool upper, bool &overflow,
bool negate);
BoolNode* rc_predicate(IdealLoopTree* loop, Node* ctrl, int scale, Node* offset, Node* init, Node* limit,
jint stride, Node* range, bool upper, bool& overflow);
// Implementation of the loop predication to promote checks outside the loop
bool loop_predication_impl(IdealLoopTree *loop);
bool loop_predication_impl_helper(IdealLoopTree* loop, IfProjNode* if_proj,
bool loop_predication_impl_helper(IdealLoopTree* loop, IfProjNode* if_success_proj,
ParsePredicateSuccessProj* parse_predicate_proj, CountedLoopNode* cl, ConNode* zero,
Invariance& invar, Deoptimization::DeoptReason reason);
bool loop_predication_should_follow_branches(IdealLoopTree* loop, IfProjNode* predicate_proj, float& loop_trip_cnt);

182
test/hotspot/jtreg/compiler/predicates/TestHoistedPredicateForNonRangeCheck.java Normal file
View File

@ -0,0 +1,182 @@
/*
* Copyright (c) 2023, 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 8307683
* @library /test/lib /
* @requires vm.compiler2.enabled
* @summary Tests that IfNode is not wrongly chosen as range check by Loop Predication leading to crashes and wrong executions.
* @run main/othervm -Xcomp -XX:CompileCommand=compileonly,compiler.predicates.TestHoistedPredicateForNonRangeCheck::test*
* compiler.predicates.TestHoistedPredicateForNonRangeCheck
* @run main/othervm -Xcomp -XX:CompileCommand=compileonly,compiler.predicates.TestHoistedPredicateForNonRangeCheck::test*
* -XX:LoopMaxUnroll=0 compiler.predicates.TestHoistedPredicateForNonRangeCheck
*/
/*
* @test
* @bug 8307683
* @library /test/lib /
* @summary Tests that IfNode is not wrongly chosen as range check by Loop Predication leading to crashes and wrong executions.
* @run main/othervm -Xbatch compiler.predicates.TestHoistedPredicateForNonRangeCheck calendar
*/
package compiler.predicates;
import jdk.test.lib.Asserts;
import java.util.Calendar;
import java.util.Date;
public class TestHoistedPredicateForNonRangeCheck {
static int iFld, iFld2;
static int[] iArr = new int[100];
public static void main(String[] args) {
if (args.length == 0) {
Integer.compareUnsigned(34, 34); // Ensure Integer class is loaded and we do not emit a trap inside test() for it.
for (int i = 0; i < 2; i++) {
iFld = 0;
iFld2 = 0;
test();
Asserts.assertEQ(iFld, 3604, "wrong value");
Asserts.assertEQ(iFld2, 400, "wrong value");
}
for (int i = 0; i < 2000; i++) {
iFld = -100;
testRangeCheckNode();
}
iFld = -1;
iFld2 = 0;
testRangeCheckNode();
Asserts.assertEQ(iFld2, 36, "wrong value");
} else {
boolean flag = false;
for (int i = 0; i < 10000; i++) {
testCalendar1();
testCalendar2(flag);
}
}
}
public static void test() {
for (int i = -1; i < 1000; i++) {
// We hoist this check and insert a Hoisted Predicate for the lower and upper bound:
// -1 >=u 100 && 1000 >= u 100 -> always true and the predicates are removed.
// Template Assertion Predicates, however, are kept. When splitting this loop further, we insert an Assertion
// Predicate which fails for i = 0 and we halt.
// When not splitting this loop (with LoopMaxUnroll=0), we have a wrong execution due to never executing
// iFld2++ (we remove the check and the branch with the trap when creating the Hoisted Predicates).
if (Integer.compareUnsigned(i, 100) < 0) {
iFld2++;
Float.isNaN(34); // Float class is unloaded with -Xcomp -> inserts trap
} else {
iFld++;
}
// Same but flipped condition and moved trap to other branch - result is the same.
if (Integer.compareUnsigned(i, 100) >= 0) { // Loop Predication creates a Hoisted Range Check Predicate due to trap with Float.isNan().
iFld++;
} else {
iFld2++;
Float.isNaN(34); // Float class is unloaded with -Xcomp -> inserts trap
}
// Same but with LoadRangeNode.
if (Integer.compareUnsigned(i, iArr.length) >= 0) { // Loop Predication creates a Hoisted Range Check Predicate due to trap with Float.isNan().
iFld++;
} else {
iFld2++;
Float.isNaN(34); // Float class is unloaded with -Xcomp -> inserts trap
}
// Same but with LoadRangeNode and flipped condition and moved trap to other branch - result is the same.
if (Integer.compareUnsigned(i, iArr.length) >= 0) { // Loop Predication creates a Hoisted Range Check Predicate due to trap with Float.isNan().
iFld++;
} else {
iFld2++;
Float.isNaN(34); // Float class is unloaded with -Xcomp -> inserts trap
}
}
}
static void testRangeCheckNode() {
int array[] = new int[34];
// Hoisted Range Check Predicate with flipped bool because trap is on success proj and no trap on false proj due
// to catching exception:
// iFld >=u 34 && iFld+36 >=u 34
// This is always false for first 2000 iterations where, initially, iFld = -100
// It is still true in the last iteration where, initially, iFld = -1. But suddenly, in the second iteration,
// where iFld = 0, we would take the true projection for the first time - but we removed that branch when
// creating the Hoisted Range Check Predicate. We therefore run into the same problem as with test(): We either
// halt due to Assertion Predicates catching this case or we have a wrong execution (iFld2 never updated).
for (int i = 0; i < 37; i++) {
try {
array[iFld] = 34; // Normal RangeCheckNode
iFld2++;
Math.ceil(34); // Never taken and unloaded -> trap
} catch (Exception e) {
// False Proj of RangeCheckNode
iFld++;
}
}
}
// Reported in JDK-8307683
static void testCalendar1() {
Calendar c = Calendar.getInstance();
c.setLenient(false);
c.set(Calendar.HOUR_OF_DAY, 0);
c.set(Calendar.MINUTE, 0);
c.getTime();
}
// Reported in JDK-8307978
static void testCalendar2(boolean flag) {
flag = !flag;
Calendar timespan = removeTime(new Date(), flag);
timespan.getTime();
}
static Calendar removeTime(Date date, boolean flag) {
Calendar calendar = Calendar.getInstance();
if (flag) {
calendar.setLenient(false);
}
calendar.setTime(date);
calendar = removeTime(calendar);
return calendar;
}
static Calendar removeTime(Calendar calendar) {
calendar.set(Calendar.HOUR_OF_DAY, 0);
calendar.set(Calendar.MINUTE, 0);
calendar.set(Calendar.SECOND, 0);
calendar.set(Calendar.MILLISECOND, 0);
return calendar;
}
}