diff --git a/src/hotspot/share/opto/divnode.cpp b/src/hotspot/share/opto/divnode.cpp index e1b143f65f8cb..40c953389d117 100644 --- a/src/hotspot/share/opto/divnode.cpp +++ b/src/hotspot/share/opto/divnode.cpp @@ -1474,6 +1474,116 @@ const Type* ModDNode::Value(PhaseGVN* phase) const { return TypeD::make(jdouble_cast(xr)); } +// If the division control input is a test that excludes 0 from the divisor value range, then the +// division depends only on that test. This check is pessimistic, that means that it may return +// false even if the control input is a test that excludes 0 from the divisor value range +template +static bool div_mod_depends_only_on_test(const Node* division) { + static_assert(std::is_same::value || std::is_same::value, "must be int or long"); + + BasicType bt; + if (std::is_same::value) { + bt = T_INT; + } else { + bt = T_LONG; + } + + Node* control = division->in(0); + if (control == nullptr) { + return true; + } + + // The control input is not a test + if (!control->is_IfTrue() && !control->is_IfFalse()) { + return false; + } + Node* iff = control->in(0); + if (!iff->is_If()) { + return false; + } + BoolNode* bol = iff->in(1)->isa_Bool(); + if (bol == nullptr) { + return false; + } + Node* cmp = bol->in(1); + if (cmp->Opcode() != Op_Cmp(bt) && cmp->Opcode() != Op_Cmp_unsigned(bt)) { + return false; + } + + bool is_unsigned = cmp->Opcode() == Op_Cmp_unsigned(bt); + bool is_reverted = control->is_IfFalse(); + bool is_commuted; + Node* divisor = division->in(2)->uncast(); + Node* other; + if (cmp->in(1)->uncast() == divisor) { + other = cmp->in(2); + is_commuted = false; + } else if (cmp->in(2)->uncast() == divisor) { + other = cmp->in(1); + is_commuted = true; + } else { + // Not a test involving the divisor + return false; + } + + // Normalize to the form (divisor <=> other) == true + BoolTest normalized_test = bol->_test; + if (is_reverted) { + normalized_test = normalized_test.negate(); + } + if (is_commuted) { + normalized_test = normalized_test.commute(); + } + + // Try to see if the test does exclude 0 from the divisor value range + if (other->bottom_type() == Type::TOP) { + return false; + } + const TypeClass* bottom_type = other->bottom_type()->cast(); + switch (normalized_test._test) { + case BoolTest::eq: return bottom_type->_lo > 0 || bottom_type->_hi < 0; + case BoolTest::ne: return bottom_type == TypeClass::ZERO; + case BoolTest::lt: return !is_unsigned && bottom_type->_hi <= 0; + case BoolTest::le: return !is_unsigned && bottom_type->_hi < 0; + case BoolTest::gt: return is_unsigned || bottom_type->_lo >= 0; + case BoolTest::ge: return bottom_type->_lo > 0; + default: ShouldNotReachHere(); + } + return false; +} + +bool DivINode::depends_only_on_test() const { + return div_mod_depends_only_on_test(this); +} + +bool DivLNode::depends_only_on_test() const { + return div_mod_depends_only_on_test(this); +} + +bool UDivINode::depends_only_on_test() const { + return div_mod_depends_only_on_test(this); +} + +bool UDivLNode::depends_only_on_test() const { + return div_mod_depends_only_on_test(this); +} + +bool ModINode::depends_only_on_test() const { + return div_mod_depends_only_on_test(this); +} + +bool ModLNode::depends_only_on_test() const { + return div_mod_depends_only_on_test(this); +} + +bool UModINode::depends_only_on_test() const { + return div_mod_depends_only_on_test(this); +} + +bool UModLNode::depends_only_on_test() const { + return div_mod_depends_only_on_test(this); +} + //============================================================================= DivModNode::DivModNode( Node *c, Node *dividend, Node *divisor ) : MultiNode(3) { diff --git a/src/hotspot/share/opto/divnode.hpp b/src/hotspot/share/opto/divnode.hpp index b9a4a32d156e9..5214798a0b380 100644 --- a/src/hotspot/share/opto/divnode.hpp +++ b/src/hotspot/share/opto/divnode.hpp @@ -49,6 +49,7 @@ class DivINode : public Node { virtual const Type* Value(PhaseGVN* phase) const; virtual const Type *bottom_type() const { return TypeInt::INT; } virtual uint ideal_reg() const { return Op_RegI; } + virtual bool depends_only_on_test() const; }; //------------------------------DivLNode--------------------------------------- @@ -62,6 +63,7 @@ class DivLNode : public Node { virtual const Type* Value(PhaseGVN* phase) const; virtual const Type *bottom_type() const { return TypeLong::LONG; } virtual uint ideal_reg() const { return Op_RegL; } + virtual bool depends_only_on_test() const; }; //------------------------------DivFNode--------------------------------------- @@ -101,6 +103,7 @@ class UDivINode : public Node { virtual Node *Ideal(PhaseGVN *phase, bool can_reshape); virtual const Type *bottom_type() const { return TypeInt::INT; } virtual uint ideal_reg() const { return Op_RegI; } + virtual bool depends_only_on_test() const; }; //------------------------------UDivLNode--------------------------------------- @@ -114,6 +117,7 @@ class UDivLNode : public Node { virtual Node *Ideal(PhaseGVN *phase, bool can_reshape); virtual const Type *bottom_type() const { return TypeLong::LONG; } virtual uint ideal_reg() const { return Op_RegL; } + virtual bool depends_only_on_test() const; }; //------------------------------ModINode--------------------------------------- @@ -126,6 +130,7 @@ class ModINode : public Node { virtual Node *Ideal(PhaseGVN *phase, bool can_reshape); virtual const Type *bottom_type() const { return TypeInt::INT; } virtual uint ideal_reg() const { return Op_RegI; } + virtual bool depends_only_on_test() const; }; //------------------------------ModLNode--------------------------------------- @@ -138,6 +143,7 @@ class ModLNode : public Node { virtual Node *Ideal(PhaseGVN *phase, bool can_reshape); virtual const Type *bottom_type() const { return TypeLong::LONG; } virtual uint ideal_reg() const { return Op_RegL; } + virtual bool depends_only_on_test() const; }; //------------------------------ModFNode--------------------------------------- @@ -172,6 +178,7 @@ class UModINode : public Node { virtual const Type *bottom_type() const { return TypeInt::INT; } virtual uint ideal_reg() const { return Op_RegI; } virtual const Type* Value(PhaseGVN* phase) const; + virtual bool depends_only_on_test() const; }; //------------------------------UModLNode--------------------------------------- @@ -184,6 +191,7 @@ class UModLNode : public Node { virtual const Type *bottom_type() const { return TypeLong::LONG; } virtual uint ideal_reg() const { return Op_RegL; } virtual const Type* Value(PhaseGVN* phase) const; + virtual bool depends_only_on_test() const; }; //------------------------------DivModNode--------------------------------------- @@ -203,6 +211,7 @@ class DivModNode : public MultiNode { virtual uint hash() const { return Node::hash(); } virtual bool is_CFG() const { return false; } virtual uint ideal_reg() const { return NotAMachineReg; } + virtual bool depends_only_on_test() const { return false; } static DivModNode* make(Node* div_or_mod, BasicType bt, bool is_unsigned); diff --git a/src/hotspot/share/opto/ifnode.cpp b/src/hotspot/share/opto/ifnode.cpp index 3c50e7993c208..ef187a9cf27f4 100644 --- a/src/hotspot/share/opto/ifnode.cpp +++ b/src/hotspot/share/opto/ifnode.cpp @@ -1569,9 +1569,8 @@ Node* IfNode::dominated_by(Node* prev_dom, PhaseIterGVN* igvn, bool pin_array_ac // Loop ends when projection has no more uses. for (DUIterator_Last jmin, j = ifp->last_outs(jmin); j >= jmin; --j) { Node* s = ifp->last_out(j); // Get child of IfTrue/IfFalse - if (s->depends_only_on_test() && igvn->no_dependent_zero_check(s)) { - // For control producers. - // Do not rewire Div and Mod nodes which could have a zero divisor to avoid skipping their zero check. + if (s->depends_only_on_test()) { + // For control producers igvn->replace_input_of(s, 0, data_target); // Move child to data-target if (pin_array_access_nodes && data_target != top) { // As a result of range check smearing, Loads and range check Cast nodes that are control dependent on this diff --git a/src/hotspot/share/opto/loopopts.cpp b/src/hotspot/share/opto/loopopts.cpp index 90b9bc84def14..86a5c3d2712ba 100644 --- a/src/hotspot/share/opto/loopopts.cpp +++ b/src/hotspot/share/opto/loopopts.cpp @@ -363,8 +363,7 @@ void PhaseIdealLoop::rewire_safe_outputs_to_dominator(Node* source, Node* domina for (DUIterator_Fast imax, i = source->fast_outs(imax); i < imax; i++) { Node* out = source->fast_out(i); // Control-dependent node - // Do not rewire Div and Mod nodes which could have a zero divisor to avoid skipping their zero check. - if (out->depends_only_on_test() && _igvn.no_dependent_zero_check(out)) { + if (out->depends_only_on_test()) { assert(out->in(0) == source, "must be control dependent on source"); _igvn.replace_input_of(out, 0, dominator); if (pin_array_access_nodes) { @@ -1648,7 +1647,7 @@ bool PhaseIdealLoop::try_merge_identical_ifs(Node* n) { void PhaseIdealLoop::push_pinned_nodes_thru_region(IfNode* dom_if, Node* region) { for (DUIterator i = region->outs(); region->has_out(i); i++) { Node* u = region->out(i); - if (!has_ctrl(u) || u->is_Phi() || !u->depends_only_on_test() || !_igvn.no_dependent_zero_check(u)) { + if (!has_ctrl(u) || u->is_Phi() || !u->depends_only_on_test()) { continue; } assert(u->in(0) == region, "not a control dependent node?"); diff --git a/src/hotspot/share/opto/node.hpp b/src/hotspot/share/opto/node.hpp index bb95601f9e7c4..88083a8e9e610 100644 --- a/src/hotspot/share/opto/node.hpp +++ b/src/hotspot/share/opto/node.hpp @@ -1035,12 +1035,43 @@ class Node { virtual bool is_CFG() const { return false; } - // If this node is control-dependent on a test, can it be - // rerouted to a dominating equivalent test? This is usually - // true of non-CFG nodes, but can be false for operations which - // depend for their correct sequencing on more than one test. - // (In that case, hoisting to a dominating test may silently - // skip some other important test.) + // If this node is control-dependent on a test, can it be rerouted to a dominating equivalent + // test? This means that the node can be executed safely as long as it happens after the test + // that is its control input without worrying about the whole control flow. On the contrary, if + // the node depends on a test that is not its control input, or if it depends on more than one + // tests, then this method must return false. + // + // Pseudocode examples: + // 1. if (y != 0) { + // x / y; + // } + // The division depends only on the test y != 0 and can be executed anywhere y != 0 holds true. + // As a result, depends_only_on_test returns true. + // 2. if (y != 0) { + // if (x > 1) { + // x / y; + // } + // } + // If the division x / y has its control input being the IfTrueNode of the test y != 0, then + // depends_only_on_test returns true. Otherwise, if the division has its control input being the + // IfTrueNode of the test x > 1, then depends_only_on_test returns false. + // 3. if (y > z) { + // if (z > 0) { + // x / y + // } + // } + // The division depends on both tests y > z and z > 0. As a result, depends_only_on_test returns + // false. + // + // This method allows more freedom in certain nodes with regards to scheduling, for example it + // allows nodes to float out of loops together with its test. + // + // This method is pessimistic, this means that it may return false even if the node satisfy its + // requirements. However, it must return false if the node does not satisfy its requirements. + // When a test is decomposed into multiple tests, all nodes that depend on the decomposed test + // must be pinned at the common denominator of those tests. For example, when a zero check of a + // division is split through a region but the division itself is not, it must be pinned at the + // merge point by returning false when calling this method. virtual bool depends_only_on_test() const { assert(!is_CFG(), ""); return true; }; // When building basic blocks, I need to have a notion of block beginning diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index dc4a27d8351fb..0da46aa340bfd 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -1727,37 +1727,6 @@ void PhaseIterGVN::remove_speculative_types() { _table.check_no_speculative_types(); } -// Check if the type of a divisor of a Div or Mod node includes zero. -bool PhaseIterGVN::no_dependent_zero_check(Node* n) const { - switch (n->Opcode()) { - case Op_DivI: - case Op_ModI: - case Op_UDivI: - case Op_UModI: { - // Type of divisor includes 0? - if (type(n->in(2)) == Type::TOP) { - // 'n' is dead. Treat as if zero check is still there to avoid any further optimizations. - return false; - } - const TypeInt* type_divisor = type(n->in(2))->is_int(); - return (type_divisor->_hi < 0 || type_divisor->_lo > 0); - } - case Op_DivL: - case Op_ModL: - case Op_UDivL: - case Op_UModL: { - // Type of divisor includes 0? - if (type(n->in(2)) == Type::TOP) { - // 'n' is dead. Treat as if zero check is still there to avoid any further optimizations. - return false; - } - const TypeLong* type_divisor = type(n->in(2))->is_long(); - return (type_divisor->_hi < 0 || type_divisor->_lo > 0); - } - } - return true; -} - //============================================================================= #ifndef PRODUCT uint PhaseCCP::_total_invokes = 0; diff --git a/src/hotspot/share/opto/phaseX.hpp b/src/hotspot/share/opto/phaseX.hpp index 7843bd39aeaab..57f43d5e589b5 100644 --- a/src/hotspot/share/opto/phaseX.hpp +++ b/src/hotspot/share/opto/phaseX.hpp @@ -582,7 +582,6 @@ class PhaseIterGVN : public PhaseGVN { } bool is_dominator(Node *d, Node *n) { return is_dominator_helper(d, n, false); } - bool no_dependent_zero_check(Node* n) const; #ifndef PRODUCT static bool is_verify_def_use() { diff --git a/test/hotspot/jtreg/compiler/loopopts/TestLoopPredicationDivZeroCheck.java b/test/hotspot/jtreg/compiler/loopopts/TestLoopPredicationDivZeroCheck.java new file mode 100644 index 0000000000000..c4a6e29f96d57 --- /dev/null +++ b/test/hotspot/jtreg/compiler/loopopts/TestLoopPredicationDivZeroCheck.java @@ -0,0 +1,88 @@ +/* + * Copyright (c) 2024, 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 8331717 + * @summary C2: Crash with SIGFPE + * + * @run main/othervm -XX:CompileCommand=compileonly,*TestLoopPredicationDivZeroCheck*::* -XX:-TieredCompilation -Xbatch TestLoopPredicationDivZeroCheck + */ + +public class TestLoopPredicationDivZeroCheck { + static int iArr[] = new int[100]; + static volatile long lFld; + static int iFld; + + public static void main(String[] strArr) { + for (int i = 0; i < 10000; i++) { + test(); + } + for (int i = 0; i < 10000; i++) { + test2(); + } + } + + /* + * The division 2 / i4 requires a non-zero check. As the result is an array access, it will be the input to a range + * check. Loop predication will try to move the range check and the division to right before the loop as the division + * appears to be invariant (i4 is always 0). However, the division is not truly invariant as it requires the zero + * check for i4 that can throw an exception. The bug fixed in 8331717 caused the division to still be moved before the + * for loop with the range check. + */ + static void test() { + int i1 = iFld; + for (int i4 : iArr) { + i4 = i1; + try { + iArr[0] = 1 / i4; + i4 = iArr[2 / i4]; + } catch (ArithmeticException a_e) { + } + } + } + + /* + * Loop predication will try to move 3 / y (input to the range check for bArr[x / 30]) before its containing for loop + * but it may not as y must be zero-checked. The same problem as above occurred before the fix in 8331717. + */ + static void test2() { + int x = 0; + int y = iFld; + long lArr[] = new long[400]; + boolean bArr[] = new boolean[400]; + for (int i = 0; i < 10000; i++) { + for (int j = 1; j < 13; j++) { + for (int k = 1; k < 2; k++) { + lFld = 0; + lArr[1] = 7; + try { + x = 3 / y; + } catch (ArithmeticException a_e) { + } + bArr[x / 30] = true; + } + } + } + } +}