8136461: PhaseIdealLoop::try_move_store_before_loop() may bypass early loop exit
Summary: PhaseIdealLoop::try_move_store_before_loop() needs to check for early loop exit before candidate Stores
Reviewed-by: kvn
--- a/hotspot/src/share/vm/opto/loopopts.cpp Tue Sep 15 12:59:51 2015 +0000
+++ b/hotspot/src/share/vm/opto/loopopts.cpp Tue Sep 15 13:08:43 2015 +0200
@@ -653,7 +653,6 @@
return iff->in(1);
}
-#ifdef ASSERT
static void enqueue_cfg_uses(Node* m, Unique_Node_List& wq) {
for (DUIterator_Fast imax, i = m->fast_outs(imax); i < imax; i++) {
Node* u = m->fast_out(i);
@@ -667,7 +666,6 @@
}
}
}
-#endif
// Try moving a store out of a loop, right before the loop
Node* PhaseIdealLoop::try_move_store_before_loop(Node* n, Node *n_ctrl) {
@@ -687,11 +685,15 @@
// written at iteration i by the second store could be overwritten
// at iteration i+n by the first store: it's not safe to move the
// first store out of the loop
- // - nothing must observe the Phi memory: it guarantees no read
- // before the store and no early exit out of the loop
- // With those conditions, we are also guaranteed the store post
- // dominates the loop head. Otherwise there would be extra Phi
- // involved between the loop's Phi and the store.
+ // - nothing must observe the memory Phi: it guarantees no read
+ // before the store, we are also guaranteed the store post
+ // dominates the loop head (ignoring a possible early
+ // exit). Otherwise there would be extra Phi involved between the
+ // loop's Phi and the store.
+ // - there must be no early exit from the loop before the Store
+ // (such an exit most of the time would be an extra use of the
+ // memory Phi but sometimes is a bottom memory Phi that takes the
+ // store as input).
if (!n_loop->is_member(address_loop) &&
!n_loop->is_member(value_loop) &&
@@ -699,9 +701,10 @@
mem->outcnt() == 1 &&
mem->in(LoopNode::LoopBackControl) == n) {
-#ifdef ASSERT
- // Verify that store's control does post dominate loop entry and
- // that there's no early exit of the loop before the store.
+ assert(n_loop->_tail != NULL, "need a tail");
+ assert(is_dominator(n_ctrl, n_loop->_tail), "store control must not be in a branch in the loop");
+
+ // Verify that there's no early exit of the loop before the store.
bool ctrl_ok = false;
{
// Follow control from loop head until n, we exit the loop or
@@ -709,7 +712,7 @@
ResourceMark rm;
Unique_Node_List wq;
wq.push(n_loop->_head);
- assert(n_loop->_tail != NULL, "need a tail");
+
for (uint next = 0; next < wq.size(); ++next) {
Node *m = wq.at(next);
if (m == n->in(0)) {
@@ -722,24 +725,27 @@
break;
}
enqueue_cfg_uses(m, wq);
+ if (wq.size() > 10) {
+ ctrl_ok = false;
+ break;
+ }
}
}
- assert(ctrl_ok, "bad control");
-#endif
+ if (ctrl_ok) {
+ // move the Store
+ _igvn.replace_input_of(mem, LoopNode::LoopBackControl, mem);
+ _igvn.replace_input_of(n, 0, n_loop->_head->in(LoopNode::EntryControl));
+ _igvn.replace_input_of(n, MemNode::Memory, mem->in(LoopNode::EntryControl));
+ // Disconnect the phi now. An empty phi can confuse other
+ // optimizations in this pass of loop opts.
+ _igvn.replace_node(mem, mem->in(LoopNode::EntryControl));
+ n_loop->_body.yank(mem);
- // move the Store
- _igvn.replace_input_of(mem, LoopNode::LoopBackControl, mem);
- _igvn.replace_input_of(n, 0, n_loop->_head->in(LoopNode::EntryControl));
- _igvn.replace_input_of(n, MemNode::Memory, mem->in(LoopNode::EntryControl));
- // Disconnect the phi now. An empty phi can confuse other
- // optimizations in this pass of loop opts.
- _igvn.replace_node(mem, mem->in(LoopNode::EntryControl));
- n_loop->_body.yank(mem);
+ IdealLoopTree* new_loop = get_loop(n->in(0));
+ set_ctrl_and_loop(n, n->in(0));
- IdealLoopTree* new_loop = get_loop(n->in(0));
- set_ctrl_and_loop(n, n->in(0));
-
- return n;
+ return n;
+ }
}
}
return NULL;
--- a/hotspot/test/compiler/TestMoveStoresOutOfLoopsStoreNoCtrl.java Tue Sep 15 12:59:51 2015 +0000
+++ /dev/null Thu Jan 01 00:00:00 1970 +0000
@@ -1,49 +0,0 @@
-/*
- * 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 8134288
- * @summary Store nodes may not have a control if used to update profiling
- * @run main/othervm -XX:-ProfileInterpreter -XX:-TieredCompilation -XX:-BackgroundCompilation TestMoveStoresOutOfLoopsStoreNoCtrl
- *
- */
-
-public class TestMoveStoresOutOfLoopsStoreNoCtrl {
-
- static void test(boolean flag) {
- for (int i = 0; i < 20000; i++) {
- if (flag) {
- int j = 0;
- do {
- j++;
- } while(j < 10);
- }
- }
- }
-
- static public void main(String[] args) {
- test(false);
- }
-
-}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/loopopts/TestMoveStoresOutOfLoopsStoreNoCtrl.java Tue Sep 15 13:08:43 2015 +0200
@@ -0,0 +1,49 @@
+/*
+ * 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 8134288
+ * @summary Store nodes may not have a control if used to update profiling
+ * @run main/othervm -XX:-ProfileInterpreter -XX:-TieredCompilation -XX:-BackgroundCompilation TestMoveStoresOutOfLoopsStoreNoCtrl
+ *
+ */
+
+public class TestMoveStoresOutOfLoopsStoreNoCtrl {
+
+ static void test(boolean flag) {
+ for (int i = 0; i < 20000; i++) {
+ if (flag) {
+ int j = 0;
+ do {
+ j++;
+ } while(j < 10);
+ }
+ }
+ }
+
+ static public void main(String[] args) {
+ test(false);
+ }
+
+}