--- old/src/hotspot/share/opto/cfgnode.cpp 2020-07-15 11:49:41.047714560 +0200 +++ new/src/hotspot/share/opto/cfgnode.cpp 2020-07-15 11:49:40.611529333 +0200 @@ -1080,9 +1080,9 @@ if (bt != BoolTest::ne) { if (stride_t->_hi < 0) { // Down-counter loop swap(lo, hi); - return TypeInt::make(MIN2(lo->_lo, hi->_lo) , hi->_hi, 3); + return TypeInt::make(MIN2(lo->_lo, hi->_lo) , hi->_hi, 3)->filter_speculative(_type); } else if (stride_t->_lo >= 0) { - return TypeInt::make(lo->_lo, MAX2(lo->_hi, hi->_hi), 3); + return TypeInt::make(lo->_lo, MAX2(lo->_hi, hi->_hi), 3)->filter_speculative(_type); } } } --- old/src/hotspot/share/opto/loopnode.hpp 2020-07-15 11:49:42.232217561 +0200 +++ new/src/hotspot/share/opto/loopnode.hpp 2020-07-15 11:49:41.784027236 +0200 @@ -1352,6 +1352,7 @@ void try_move_store_after_loop(Node* n); bool identical_backtoback_ifs(Node *n); bool can_split_if(Node *n_ctrl); + DEBUG_ONLY(void check_divide_by_zero(const Node* n, const Type* zero) const;) // Determine if a method is too big for a/another round of split-if, based on // a magic (approximate) ratio derived from the equally magic constant 35000, --- old/src/hotspot/share/opto/loopopts.cpp 2020-07-15 11:49:43.160611806 +0200 +++ new/src/hotspot/share/opto/loopopts.cpp 2020-07-15 11:49:42.764443572 +0200 @@ -46,7 +46,7 @@ //============================================================================= //------------------------------split_thru_phi--------------------------------- // Split Node 'n' through merge point if there is enough win. -Node *PhaseIdealLoop::split_thru_phi( Node *n, Node *region, int policy ) { +Node* PhaseIdealLoop::split_thru_phi(Node* n, Node* region, int policy) { if (n->Opcode() == Op_ConvI2L && n->bottom_type() != TypeLong::LONG) { // ConvI2L may have type information on it which is unsafe to push up // so disable this for now @@ -61,13 +61,40 @@ return NULL; } + // Bail out if 'n' is a Div or Mod node whose zero check was removed earlier (i.e. control is NULL) and its divisor is a phi node d_phi whose + // inputs could be zero (include zero in their type range). This can only happen if d_phi is an induction variable of a trip-counted (integer) + // loop. d_phi could have a more precise type range that does not necessarily include all values of its inputs. Since each of these inputs will + // be a divisor of the newly cloned nodes of 'n', we need to bail out of one of these divisors could be zero (zero in its type range). + if ((n->Opcode() == Op_DivI || n->Opcode() == Op_ModI) && n->in(0) == NULL + && region->is_CountedLoop() && n->in(2) == region->as_CountedLoop()->phi()) { + Node* phi = region->as_CountedLoop()->phi(); + for (uint i = 1; i < phi->req(); i++) { + if (_igvn.type(phi->in(i))->filter_speculative(TypeInt::ZERO) != Type::TOP) { + // Zero could be a possible value but we already removed the zero check. Bail out to avoid a possible division by zero at a later point. + return NULL; + } + } + } + +#ifdef ASSERT + // A phi only has a more precise type than its inputs if it is an induction variable of a trip-counted integer loop. Verify that this is the + // case and we are not about to split a Mod or Div node without zero check that could end up with a new divisor that can be zero. + // A split of a division by a floating point zero, on the other hand, is not a problem since the result would just be INFINITY as floating point + // representation and does not result in an exception. + if ((n->Opcode() == Op_DivI || n->Opcode() == Op_ModI)) { + check_divide_by_zero(n, TypeInt::ZERO); + } else if ((n->Opcode() == Op_DivL || n->Opcode() == Op_ModL)) { + check_divide_by_zero(n, TypeLong::ZERO); + } +#endif + int wins = 0; assert(!n->is_CFG(), ""); assert(region->is_Region(), ""); const Type* type = n->bottom_type(); - const TypeOopPtr *t_oop = _igvn.type(n)->isa_oopptr(); - Node *phi; + const TypeOopPtr* t_oop = _igvn.type(n)->isa_oopptr(); + Node* phi; if (t_oop != NULL && t_oop->is_known_instance_field()) { int iid = t_oop->instance_id(); int index = C->get_alias_index(t_oop); @@ -78,7 +105,7 @@ } uint old_unique = C->unique(); for (uint i = 1; i < region->req(); i++) { - Node *x; + Node* x; Node* the_clone = NULL; if (region->in(i) == C->top()) { x = C->top(); // Dead path? Use a dead data op @@ -89,13 +116,13 @@ if (n->in(0) == region) x->set_req( 0, region->in(i) ); for (uint j = 1; j < n->req(); j++) { - Node *in = n->in(j); + Node* in = n->in(j); if (in->is_Phi() && in->in(0) == region) - x->set_req( j, in->in(i) ); // Use pre-Phi input for the clone + x->set_req(j, in->in(i)); // Use pre-Phi input for the clone } } // Check for a 'win' on some paths - const Type *t = x->Value(&_igvn); + const Type* t = x->Value(&_igvn); bool singleton = t->singleton(); @@ -210,6 +237,22 @@ return phi; } +#ifdef ASSERT +void PhaseIdealLoop::check_divide_by_zero(const Node* n, const Type* zero) const { + if (n->in(0) == NULL && n->in(2)->is_Phi()) { + Node* phi = n->in(2); + for (uint i = 1; i < phi->req(); i++) { + if (_igvn.type(phi->in(i))->filter_speculative(zero) != Type::TOP) { + // Zero could be a possible value + n->dump(2); + phi->dump(2); + assert(false, "should not happen that pre-phi type includes zero as possible value but zero check was already removed"); + } + } + } +} +#endif + //------------------------------dominated_by------------------------------------ // Replace the dominated test with an obvious true or false. Place it on the // IGVN worklist for later cleanup. Move control-dependent data Nodes on the --- /dev/null 2020-07-13 13:24:27.056607695 +0200 +++ new/test/hotspot/jtreg/compiler/loopopts/TestSplitThruPhiDivMod.java 2020-07-15 11:49:43.856907489 +0200 @@ -0,0 +1,95 @@ +/* + * Copyright (c) 2020, 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 8248552 + * @summary A Division/modulo node whose zero check was removed is split through an induction variable phi and executed before + * the loop limit check resulting in a SIGFPE because the divisor is zero. + * + * @run main/othervm -XX:CompileCommand=dontinline,compiler.c2.loopopts.TestSplitThruPhiDivMod::test* compiler.c2.loopopts.TestSplitThruPhiDivMod + */ +package compiler.c2.loopopts; + +public class TestSplitThruPhiDivMod { + + int x; + + public int testMod() { + int i1 = 2; + for (int i = 5; i < 25; i++) { + for (int j = 50; j > 1; j -= 2) { + /* + * Zero check is removed based on the type of the induction variable phi (variable j) since its always between 1 and 50. + * However, when splitting the modulo node through the phi, it can be executed right after the subtraction j-2 which can be + * 0 before evaluation the loop limit condition in the last iteration when j is 2: j-2 = 2-2 = 0. This results in a SIGFPE. + * The fix is to not split a division or modulo node 'n' through the induction variable phi if the zero check was removed + * earlier and the new inputs of the clones of 'n' after the split could be zero (i.e. the type of the clones of 'n' include 0). + */ + x = (20 % j); // Problematic division as part of modulo. Results in a SIGFPE, even though j is always non-zero. + i1 = (i1 / i); + for (int k = 3; k > 1; k--) { + switch ((i % 4) + 22) { + case 22: + switch (j % 10) { + case 83: + x += 5; + break; + } + } + } + } + } + return i1; + } + + public int testDiv() { + int i1 = 2; + for (int i = 5; i < 25; i++) { + for (int j = 50; j > 1; j -= 2) { + // Same issue as above but with a division node. See explanation above. + x = (20 / j); // Problematic division. Results in a SIGFPE, even though j is always non-zero. + i1 = (i1 / i); + for (int k = 3; k > 1; k--) { + switch ((i % 4) + 22) { + case 22: + switch (j % 10) { + case 83: + x += 5; + break; + } + } + } + } + } + return i1; + } + + public static void main(String[] strArr) { + TestSplitThruPhiDivMod t = new TestSplitThruPhiDivMod(); + for (int i = 0; i < 10000; i++) { + t.testDiv(); + t.testMod(); + } + } +}