8140574: C2 must re-execute checks after deoptimizing from merged uncommon traps
Summary: 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
--- a/hotspot/src/share/vm/ci/ciTypeFlow.cpp Tue Oct 27 01:45:01 2015 -0400
+++ b/hotspot/src/share/vm/ci/ciTypeFlow.cpp Fri Nov 06 09:36:47 2015 +0100
@@ -1588,6 +1588,7 @@
_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 @@
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 @@
} 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 @@
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 @@
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 @@
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;
}
}
@@ -2884,6 +2911,69 @@
}
// ------------------------------------------------------------------
+// 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.
// This is required because there is not a 1-1 relation between the ciEnv and
--- a/hotspot/src/share/vm/ci/ciTypeFlow.hpp Tue Oct 27 01:45:01 2015 -0400
+++ b/hotspot/src/share/vm/ci/ciTypeFlow.hpp Fri Nov 06 09:36:47 2015 +0100
@@ -529,6 +529,7 @@
GrowableArray<Block*>* _exceptions;
GrowableArray<ciInstanceKlass*>* _exc_klasses;
GrowableArray<Block*>* _successors;
+ GrowableArray<Block*>* _predecessors;
StateVector* _state;
JsrSet* _jsrs;
@@ -617,6 +618,12 @@
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 @@
// 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;
--- a/hotspot/src/share/vm/opto/ifnode.cpp Tue Oct 27 01:45:01 2015 -0400
+++ b/hotspot/src/share/vm/opto/ifnode.cpp Fri Nov 06 09:36:47 2015 +0100
@@ -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 @@
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 @@
} 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;
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/rangechecks/TestUncommonTrapMerging.java Fri Nov 06 09:36:47 2015 +0100
@@ -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);
+ }
+}