8140574: C2 must re-execute checks after deoptimizing from merged uncommon traps

Before merging uncommon traps we have to check for proper bci domination and compatible JVMStates to guarantee correct re-execution of the checks.

Reviewed-by: kvn, roland

hotspot/src/share/vm/ci/ciTypeFlow.cpp

@@ -1588,6 +1588,7 @@ ciTypeFlow::Block::Block(ciTypeFlow* outer,
   _exceptions = NULL;
   _exc_klasses = NULL;
   _successors = NULL;
+  _predecessors = new (outer->arena()) GrowableArray<Block*>(outer->arena(), 1, 0, NULL);
   _state = new (outer->arena()) StateVector(outer);
   JsrSet* new_jsrs =
     new (outer->arena()) JsrSet(outer->arena(), jsrs->size());
@@ -1771,6 +1772,12 @@ ciTypeFlow::Block::successors(ciBytecodeStream* str,
         break;
       }
     }
+
+    // Set predecessor information
+    for (int i = 0; i < _successors->length(); i++) {
+      Block* block = _successors->at(i);
+      block->predecessors()->append(this);
+    }
   }
   return _successors;
 }
@@ -1813,7 +1820,9 @@ void ciTypeFlow::Block::compute_exceptions() {
     } else {
       klass = handler->catch_klass();
     }
-    _exceptions->append(analyzer->block_at(bci, _jsrs));
+    Block* block = analyzer->block_at(bci, _jsrs);
+    _exceptions->append(block);
+    block->predecessors()->append(this);
     _exc_klasses->append(klass);
   }
 }
@@ -1909,6 +1918,18 @@ void ciTypeFlow::Block::print_on(outputStream* st) const {
       st->cr();
     }
   }
+  if (_predecessors == NULL) {
+    st->print_cr("  No predecessor information");
+  } else {
+    int num_predecessors = _predecessors->length();
+    st->print_cr("  Predecessors : %d", num_predecessors);
+    for (int i = 0; i < num_predecessors; i++) {
+      Block* predecessor = _predecessors->at(i);
+      st->print("    ");
+      predecessor->print_value_on(st);
+      st->cr();
+    }
+  }
   if (_exceptions == NULL) {
     st->print_cr("  No exception information");
   } else {
@@ -2270,6 +2291,9 @@ ciTypeFlow::Block* ciTypeFlow::clone_loop_head(Loop* lp, StateVector* temp_vecto
   for (SuccIter iter(tail); !iter.done(); iter.next()) {
     if (iter.succ() == head) {
       iter.set_succ(clone);
+      // Update predecessor information
+      head->predecessors()->remove(tail);
+      clone->predecessors()->append(tail);
     }
   }
   flow_block(tail, temp_vector, temp_set);
@@ -2279,6 +2303,9 @@ ciTypeFlow::Block* ciTypeFlow::clone_loop_head(Loop* lp, StateVector* temp_vecto
     for (SuccIter iter(clone); !iter.done(); iter.next()) {
       if (iter.succ() == head) {
         iter.set_succ(clone);
+        // Update predecessor information
+        head->predecessors()->remove(clone);
+        clone->predecessors()->append(clone);
         break;
       }
     }
@@ -2883,6 +2910,69 @@ void ciTypeFlow::do_flow() {
   }
 }
 
+// ------------------------------------------------------------------
+// ciTypeFlow::is_dominated_by
+//
+// Determine if the instruction at bci is dominated by the instruction at dom_bci.
+bool ciTypeFlow::is_dominated_by(int bci, int dom_bci) {
+  assert(!method()->has_jsrs(), "jsrs are not supported");
+
+  ResourceMark rm;
+  JsrSet* jsrs = new ciTypeFlow::JsrSet(NULL);
+  int        index = _methodBlocks->block_containing(bci)->index();
+  int    dom_index = _methodBlocks->block_containing(dom_bci)->index();
+  Block*     block = get_block_for(index, jsrs, ciTypeFlow::no_create);
+  Block* dom_block = get_block_for(dom_index, jsrs, ciTypeFlow::no_create);
+
+  // Start block dominates all other blocks
+  if (start_block()->rpo() == dom_block->rpo()) {
+    return true;
+  }
+
+  // Dominated[i] is true if block i is dominated by dom_block
+  int num_blocks = _methodBlocks->num_blocks();
+  bool* dominated = NEW_RESOURCE_ARRAY(bool, num_blocks);
+  for (int i = 0; i < num_blocks; ++i) {
+    dominated[i] = true;
+  }
+  dominated[start_block()->rpo()] = false;
+
+  // Iterative dominator algorithm
+  bool changed = true;
+  while (changed) {
+    changed = false;
+    // Use reverse postorder iteration
+    for (Block* blk = _rpo_list; blk != NULL; blk = blk->rpo_next()) {
+      if (blk->is_start()) {
+        // Ignore start block
+        continue;
+      }
+      // The block is dominated if it is the dominating block
+      // itself or if all predecessors are dominated.
+      int index = blk->rpo();
+      bool dom = (index == dom_block->rpo());
+      if (!dom) {
+        // Check if all predecessors are dominated
+        dom = true;
+        for (int i = 0; i < blk->predecessors()->length(); ++i) {
+          Block* pred = blk->predecessors()->at(i);
+          if (!dominated[pred->rpo()]) {
+            dom = false;
+            break;
+          }
+        }
+      }
+      // Update dominator information
+      if (dominated[index] != dom) {
+        changed = true;
+        dominated[index] = dom;
+      }
+    }
+  }
+  // block dominated by dom_block?
+  return dominated[block->rpo()];
+}
+
 // ------------------------------------------------------------------
 // ciTypeFlow::record_failure()
 // The ciTypeFlow object keeps track of failure reasons separately from the ciEnv.

hotspot/src/share/vm/ci/ciTypeFlow.hpp

@@ -529,6 +529,7 @@ public:
     GrowableArray<Block*>*           _exceptions;
     GrowableArray<ciInstanceKlass*>* _exc_klasses;
     GrowableArray<Block*>*           _successors;
+    GrowableArray<Block*>*           _predecessors;
     StateVector*                     _state;
     JsrSet*                          _jsrs;
 
@@ -617,6 +618,12 @@ public:
       return _successors;
     }
 
+    // Predecessors of this block (including exception edges)
+    GrowableArray<Block*>* predecessors() {
+      assert(_predecessors != NULL, "must be filled in");
+      return _predecessors;
+    }
+
     // Get the exceptional successors for this Block.
     GrowableArray<Block*>* exceptions() {
       if (_exceptions == NULL) {
@@ -941,6 +948,9 @@ public:
   // Perform type inference flow analysis.
   void do_flow();
 
+  // Determine if bci is dominated by dom_bci
+  bool is_dominated_by(int bci, int dom_bci);
+
   void print_on(outputStream* st) const PRODUCT_RETURN;
 
   void rpo_print_on(outputStream* st) const PRODUCT_RETURN;

hotspot/src/share/vm/opto/ifnode.cpp

@@ -23,6 +23,7 @@
  */
 
 #include "precompiled.hpp"
+#include "ci/ciTypeFlow.hpp"
 #include "memory/allocation.inline.hpp"
 #include "opto/addnode.hpp"
 #include "opto/castnode.hpp"
@@ -771,6 +772,11 @@ bool IfNode::has_only_uncommon_traps(ProjNode* proj, ProjNode*& success, ProjNod
   CallStaticJavaNode* dom_unc = otherproj->is_uncommon_trap_proj(Deoptimization::Reason_none);
 
   if (otherproj->outcnt() == 1 && dom_unc != NULL) {
+    // We need to re-execute the folded Ifs after deoptimization from the merged traps
+    if (!dom_unc->jvms()->should_reexecute()) {
+      return false;
+    }
+
     CallStaticJavaNode* unc = NULL;
     ProjNode* unc_proj = uncommon_trap_proj(unc);
     if (unc_proj != NULL && unc_proj->outcnt() == 1) {
@@ -784,12 +790,37 @@ bool IfNode::has_only_uncommon_traps(ProjNode* proj, ProjNode*& success, ProjNod
       } else if (dom_unc->in(0) != otherproj || unc->in(0) != unc_proj) {
         return false;
       }
+
+      // Different methods and methods containing jsrs are not supported.
+      ciMethod* method = unc->jvms()->method();
+      ciMethod* dom_method = dom_unc->jvms()->method();
+      if (method != dom_method || method->has_jsrs()) {
+        return false;
+      }
+      // Check that both traps are in the same activation of the method (instead
+      // of two activations being inlined through different call sites) by verifying
+      // that the call stacks are equal for both JVMStates.
+      JVMState* dom_caller = dom_unc->jvms()->caller();
+      JVMState* caller = unc->jvms()->caller();
+      if (!dom_caller->same_calls_as(caller)) {
+        return false;
+      }
+      // Check that the bci of the dominating uncommon trap dominates the bci
+      // of the dominated uncommon trap. Otherwise we may not re-execute
+      // the dominated check after deoptimization from the merged uncommon trap.
+      ciTypeFlow* flow = dom_method->get_flow_analysis();
+      int bci = unc->jvms()->bci();
+      int dom_bci = dom_unc->jvms()->bci();
+      if (!flow->is_dominated_by(bci, dom_bci)) {
+        return false;
+      }
+
       // See merge_uncommon_traps: the reason of the uncommon trap
       // will be changed and the state of the dominating If will be
       // used. Checked that we didn't apply this transformation in a
       // previous compilation and it didn't cause too many traps
-      if (!igvn->C->too_many_traps(dom_unc->jvms()->method(), dom_unc->jvms()->bci(), Deoptimization::Reason_unstable_fused_if) &&
-          !igvn->C->too_many_traps(dom_unc->jvms()->method(), dom_unc->jvms()->bci(), Deoptimization::Reason_range_check)) {
+      if (!igvn->C->too_many_traps(dom_method, dom_bci, Deoptimization::Reason_unstable_fused_if) &&
+          !igvn->C->too_many_traps(dom_method, dom_bci, Deoptimization::Reason_range_check)) {
         success = unc_proj;
         fail = unc_proj->other_if_proj();
         return true;

hotspot/test/compiler/rangechecks/TestUncommonTrapMerging.java

@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2015, 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 8140574
+ * @summary Verify proper re-execution of checks after merging of uncommon traps
+ * @run main/othervm -Xcomp -XX:-TieredCompilation -XX:CompileCommand=compileonly,TestUncommonTrapMerging::test* TestUncommonTrapMerging Test1
+ * @run main/othervm -XX:CompileCommand=compileonly,TestUncommonTrapMerging::test* TestUncommonTrapMerging Test2
+ */
+public class TestUncommonTrapMerging {
+
+    public static void main(String[] args) throws Throwable {
+        if (args.length < 1) {
+            throw new RuntimeException("Not enough arguments!");
+        }
+        TestUncommonTrapMerging mytest = new TestUncommonTrapMerging();
+        String testcase = args[0];
+        if (testcase.equals("Test1")) {
+            try {
+                // '42' should hit the 'arg > 0' check
+                mytest.test(42);
+
+            } catch (OutOfMemoryError e) {
+                // expected
+            }
+        } else if (testcase.equals("Test2")) {
+            // Compile test2 with uncommon traps at path 1 and path 2
+            for (int i = 0; i < 100_000; i++) {
+                mytest.test2(-1, 0);
+            }
+
+            // Compile test3 which inlines test2 with uncommon traps at
+            // path 1 and path 2. Because test3 always passes 'value = 1',
+            // C2 will remove the 'value > 0' check and then merge the two
+            // uncommon traps.
+            for (int i = 0; i < 100_000; i++) {
+                mytest.test3(0);
+            }
+
+            // This should return through path 2
+            if (!mytest.test3(42)) {
+                throw new RuntimeException("test2 returned through wrong path!");
+            }
+        }
+    }
+
+    public void test(int arg) throws Throwable {
+        // The following two checks should not be merged if the
+        // uncommon trap of the dominating if has 'Reason_unloaded'
+        // because we need to re-execute both checks after deopt.
+        if (arg < 0) {
+            throw new RuntimeException("Should not reach here");
+        } else if (arg > 0) {
+            throw new OutOfMemoryError();
+        }
+        throw new RuntimeException("Should not reach here");
+    }
+
+    public boolean test2(int arg, int value) {
+        if (arg < 0) {
+            if (value > 0) {
+                // path 1
+                return false;
+            }
+        } else if (arg > 0) {
+            // path 2
+            return true;
+        }
+        // path 3
+        return false;
+    }
+
+    public boolean test3(int arg) {
+        int i;
+        for (i = 0; i < 1; ++i) { }
+        // i == 1
+        return test2(arg, i);
+    }
+}