Draft: 8331717: C2: Crash with SIGFPE Because Loop Predication Wrongly Hoists Division Requiring Zero Check

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 <class TypeClass>
+static bool div_mod_depends_only_on_test(const Node* division) {
+  static_assert(std::is_same<TypeClass, TypeInt>::value || std::is_same<TypeClass, TypeLong>::value, "must be int or long");
+
+  BasicType bt;
+  if (std::is_same<TypeClass, TypeInt>::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<TypeClass>();
+  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<TypeInt>(this);
+}
+
+bool DivLNode::depends_only_on_test() const {
+  return div_mod_depends_only_on_test<TypeLong>(this);
+}
+
+bool UDivINode::depends_only_on_test() const {
+  return div_mod_depends_only_on_test<TypeInt>(this);
+}
+
+bool UDivLNode::depends_only_on_test() const {
+  return div_mod_depends_only_on_test<TypeLong>(this);
+}
+
+bool ModINode::depends_only_on_test() const {
+  return div_mod_depends_only_on_test<TypeInt>(this);
+}
+
+bool ModLNode::depends_only_on_test() const {
+  return div_mod_depends_only_on_test<TypeLong>(this);
+}
+
+bool UModINode::depends_only_on_test() const {
+  return div_mod_depends_only_on_test<TypeInt>(this);
+}
+
+bool UModLNode::depends_only_on_test() const {
+  return div_mod_depends_only_on_test<TypeLong>(this);
+}
+
 //=============================================================================
 
 DivModNode::DivModNode( Node *c, Node *dividend, Node *divisor ) : MultiNode(3) {

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

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

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?");

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

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;

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() {