8324517: C2: crash in compiled code because of dependency on removed range check CastIIs

Reviewed-by: epeter, thartmann

Author: rwestrel

src/hotspot/share/opto/castnode.cpp

@@ -213,13 +213,7 @@ const Type* CastIINode::Value(PhaseGVN* phase) const {
 
   // Similar to ConvI2LNode::Value() for the same reasons
   // see if we can remove type assertion after loop opts
-  // But here we have to pay extra attention:
-  // Do not narrow the type of range check dependent CastIINodes to
-  // avoid corruption of the graph if a CastII is replaced by TOP but
-  // the corresponding range check is not removed.
-  if (!_range_check_dependency) {
-    res = widen_type(phase, res, T_INT);
-  }
+  res = widen_type(phase, res, T_INT);
 
   return res;
 }
@@ -239,11 +233,11 @@ Node *CastIINode::Ideal(PhaseGVN *phase, bool can_reshape) {
   if (progress != nullptr) {
     return progress;
   }
-  if (can_reshape && !_range_check_dependency && !phase->C->post_loop_opts_phase()) {
+  if (can_reshape && !phase->C->post_loop_opts_phase()) {
     // makes sure we run ::Value to potentially remove type assertion after loop opts
     phase->C->record_for_post_loop_opts_igvn(this);
   }
-  if (!_range_check_dependency) {
+  if (!_type->is_int()->empty()) {
     return optimize_integer_cast(phase, T_INT);
   }
   return nullptr;
@@ -254,13 +248,6 @@ Node* CastIINode::Identity(PhaseGVN* phase) {
   if (progress != this) {
     return progress;
   }
-  if (_range_check_dependency) {
-    if (phase->C->post_loop_opts_phase()) {
-      return this->in(1);
-    } else {
-      phase->C->record_for_post_loop_opts_igvn(this);
-    }
-  }
   return this;
 }
 

src/hotspot/share/opto/compile.cpp

@@ -3464,6 +3464,10 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f
     }
     break;
   }
+  case Op_CastII: {
+    remove_range_check_cast(n->as_CastII());
+  }
+  break;
 #ifdef _LP64
   case Op_CmpP:
     // Do this transformation here to preserve CmpPNode::sub() and
@@ -3615,16 +3619,6 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f
 
 #endif
 
-#ifdef ASSERT
-  case Op_CastII:
-    // Verify that all range check dependent CastII nodes were removed.
-    if (n->isa_CastII()->has_range_check()) {
-      n->dump(3);
-      assert(false, "Range check dependent CastII node was not removed");
-    }
-    break;
-#endif
-
   case Op_ModI:
     if (UseDivMod) {
       // Check if a%b and a/b both exist
@@ -3633,6 +3627,8 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f
         // Replace them with a fused divmod if supported
         if (Matcher::has_match_rule(Op_DivModI)) {
           DivModINode* divmod = DivModINode::make(n);
+          divmod->add_prec_from(n);
+          divmod->add_prec_from(d);
           d->subsume_by(divmod->div_proj(), this);
           n->subsume_by(divmod->mod_proj(), this);
         } else {
@@ -3653,6 +3649,8 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f
         // Replace them with a fused divmod if supported
         if (Matcher::has_match_rule(Op_DivModL)) {
           DivModLNode* divmod = DivModLNode::make(n);
+          divmod->add_prec_from(n);
+          divmod->add_prec_from(d);
           d->subsume_by(divmod->div_proj(), this);
           n->subsume_by(divmod->mod_proj(), this);
         } else {
@@ -3673,6 +3671,8 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f
         // Replace them with a fused unsigned divmod if supported
         if (Matcher::has_match_rule(Op_UDivModI)) {
           UDivModINode* divmod = UDivModINode::make(n);
+          divmod->add_prec_from(n);
+          divmod->add_prec_from(d);
           d->subsume_by(divmod->div_proj(), this);
           n->subsume_by(divmod->mod_proj(), this);
         } else {
@@ -3693,6 +3693,8 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f
         // Replace them with a fused unsigned divmod if supported
         if (Matcher::has_match_rule(Op_UDivModL)) {
           UDivModLNode* divmod = UDivModLNode::make(n);
+          divmod->add_prec_from(n);
+          divmod->add_prec_from(d);
           d->subsume_by(divmod->div_proj(), this);
           n->subsume_by(divmod->mod_proj(), this);
         } else {
@@ -3894,6 +3896,34 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f
   }
 }
 
+void Compile::remove_range_check_cast(CastIINode* cast) {
+  if (cast->has_range_check()) {
+    // Range check CastII nodes feed into an address computation subgraph. Remove them to let that subgraph float freely.
+    // For memory access or integer divisions nodes that depend on the cast, record the dependency on the cast's control
+    // as a precedence edge, so they can't float above the cast in case that cast's narrowed type helped eliminate a
+    // range check or a null divisor check.
+    assert(cast->in(0) != nullptr, "All RangeCheck CastII must have a control dependency");
+    ResourceMark rm;
+    Unique_Node_List wq;
+    wq.push(cast);
+    for (uint next = 0; next < wq.size(); ++next) {
+      Node* m = wq.at(next);
+      for (DUIterator_Fast imax, i = m->fast_outs(imax); i < imax; i++) {
+        Node* use = m->fast_out(i);
+        if (use->is_Mem() || use->is_div_or_mod(T_INT) || use->is_div_or_mod(T_LONG)) {
+          use->ensure_control_or_add_prec(cast->in(0));
+        } else if (!use->is_CFG() && !use->is_Phi()) {
+          wq.push(use);
+        }
+      }
+    }
+    cast->subsume_by(cast->in(1), this);
+    if (cast->outcnt() == 0) {
+      cast->disconnect_inputs(this);
+    }
+  }
+}
+
 //------------------------------final_graph_reshaping_walk---------------------
 // Replacing Opaque nodes with their input in final_graph_reshaping_impl(),
 // requires that the walk visits a node's inputs before visiting the node.

src/hotspot/share/opto/compile.hpp

@@ -53,6 +53,7 @@ class Block;
 class Bundle;
 class CallGenerator;
 class CallStaticJavaNode;
+class CastIINode;
 class CloneMap;
 class CompilationFailureInfo;
 class ConnectionGraph;
@@ -1314,6 +1315,8 @@ class Compile : public Phase {
                             BasicType out_bt, BasicType in_bt);
 
   static Node* narrow_value(BasicType bt, Node* value, const Type* type, PhaseGVN* phase, bool transform_res);
+
+  void remove_range_check_cast(CastIINode* cast);
 };
 
 #endif // SHARE_OPTO_COMPILE_HPP

src/hotspot/share/opto/node.cpp

@@ -2878,6 +2878,15 @@ void Node::ensure_control_or_add_prec(Node* c) {
   }
 }
 
+void Node::add_prec_from(Node* n) {
+  for (uint i = n->req(); i < n->len(); i++) {
+    Node* prec = n->in(i);
+    if (prec != nullptr) {
+      add_prec(prec);
+    }
+  }
+}
+
 bool Node::is_dead_loop_safe() const {
   if (is_Phi()) {
     return true;
@@ -2901,6 +2910,9 @@ bool Node::is_dead_loop_safe() const {
   return false;
 }
 
+bool Node::is_div_or_mod(BasicType bt) const { return Opcode() == Op_Div(bt) || Opcode() == Op_Mod(bt) ||
+                                                      Opcode() == Op_UDiv(bt) || Opcode() == Op_UMod(bt); }
+
 //=============================================================================
 //------------------------------yank-------------------------------------------
 // Find and remove

src/hotspot/share/opto/node.hpp

@@ -1143,6 +1143,7 @@ class Node {
 
   // Set control or add control as precedence edge
   void ensure_control_or_add_prec(Node* c);
+  void add_prec_from(Node* n);
 
   // Visit boundary uses of the node and apply a callback function for each.
   // Recursively traverse uses, stopping and applying the callback when
@@ -1254,6 +1255,8 @@ class Node {
   // Whether this is a memory phi node
   bool is_memory_phi() const { return is_Phi() && bottom_type() == Type::MEMORY; }
 
+  bool is_div_or_mod(BasicType bt) const;
+
 //----------------- Printing, etc
 #ifndef PRODUCT
  public:
@@ -2023,6 +2026,10 @@ Op_IL(URShift)
 Op_IL(LShift)
 Op_IL(Xor)
 Op_IL(Cmp)
+Op_IL(Div)
+Op_IL(Mod)
+Op_IL(UDiv)
+Op_IL(UMod)
 
 inline int Op_ConIL(BasicType bt) {
   assert(bt == T_INT || bt == T_LONG, "only for int or longs");