diff --git a/src/hotspot/share/opto/loopPredicate.cpp b/src/hotspot/share/opto/loopPredicate.cpp index ab6dbf945cf54..35fe6a0a037f8 100644 --- a/src/hotspot/share/opto/loopPredicate.cpp +++ b/src/hotspot/share/opto/loopPredicate.cpp @@ -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(); + // } + // + // 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); diff --git a/src/hotspot/share/opto/loopTransform.cpp b/src/hotspot/share/opto/loopTransform.cpp index fb2528a1dc7c0..775299eb0c209 100644 --- a/src/hotspot/share/opto/loopTransform.cpp +++ b/src/hotspot/share/opto/loopTransform.cpp @@ -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; diff --git a/src/hotspot/share/opto/loopnode.cpp b/src/hotspot/share/opto/loopnode.cpp index f0b13817c4a88..610f87e595288 100644 --- a/src/hotspot/share/opto/loopnode.cpp +++ b/src/hotspot/share/opto/loopnode.cpp @@ -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)); diff --git a/src/hotspot/share/opto/loopnode.hpp b/src/hotspot/share/opto/loopnode.hpp index f4a9f27d73a99..548a21f568aa3 100644 --- a/src/hotspot/share/opto/loopnode.hpp +++ b/src/hotspot/share/opto/loopnode.hpp @@ -733,8 +733,8 @@ class IdealLoopTree : public ResourceObj { 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 @@ class PhaseIdealLoop : public PhaseTransform { 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); diff --git a/test/hotspot/jtreg/compiler/predicates/TestHoistedPredicateForNonRangeCheck.java b/test/hotspot/jtreg/compiler/predicates/TestHoistedPredicateForNonRangeCheck.java new file mode 100644 index 0000000000000..b9fe2e6657db7 --- /dev/null +++ b/test/hotspot/jtreg/compiler/predicates/TestHoistedPredicateForNonRangeCheck.java @@ -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; + } +}