8191887: assert(b->is_Bool()) in PhaseIdealLoop::clone_iff() due to Opaque4 node
authorroland
Mon, 27 Nov 2017 16:05:11 -0800
changeset 48135 feea6d82adc2
parent 48134 87b6295b3f4f
child 48136 c035fbb1beb4
8191887: assert(b->is_Bool()) in PhaseIdealLoop::clone_iff() due to Opaque4 node Summary: add special handling for graph shape If->Opaque4->Bool->CmpP Reviewed-by: kvn
src/hotspot/share/opto/loopnode.hpp
src/hotspot/share/opto/loopopts.cpp
test/hotspot/jtreg/compiler/unsafe/TestLoopUnswitching.java
--- a/src/hotspot/share/opto/loopnode.hpp	Mon Nov 27 17:51:21 2017 -0500
+++ b/src/hotspot/share/opto/loopnode.hpp	Mon Nov 27 16:05:11 2017 -0800
@@ -1098,7 +1098,7 @@
   // "Nearly" because all Nodes have been cloned from the original in the loop,
   // but the fall-in edges to the Cmp are different.  Clone bool/Cmp pairs
   // through the Phi recursively, and return a Bool.
-  BoolNode *clone_iff( PhiNode *phi, IdealLoopTree *loop );
+  Node *clone_iff( PhiNode *phi, IdealLoopTree *loop );
   CmpNode *clone_bool( PhiNode *phi, IdealLoopTree *loop );
 
 
--- a/src/hotspot/share/opto/loopopts.cpp	Mon Nov 27 17:51:21 2017 -0500
+++ b/src/hotspot/share/opto/loopopts.cpp	Mon Nov 27 16:05:11 2017 -0800
@@ -1407,47 +1407,56 @@
 // "Nearly" because all Nodes have been cloned from the original in the loop,
 // but the fall-in edges to the Cmp are different.  Clone bool/Cmp pairs
 // through the Phi recursively, and return a Bool.
-BoolNode *PhaseIdealLoop::clone_iff( PhiNode *phi, IdealLoopTree *loop ) {
+Node* PhaseIdealLoop::clone_iff(PhiNode *phi, IdealLoopTree *loop) {
 
   // Convert this Phi into a Phi merging Bools
   uint i;
-  for( i = 1; i < phi->req(); i++ ) {
+  for (i = 1; i < phi->req(); i++) {
     Node *b = phi->in(i);
-    if( b->is_Phi() ) {
-      _igvn.replace_input_of(phi, i, clone_iff( b->as_Phi(), loop ));
+    if (b->is_Phi()) {
+      _igvn.replace_input_of(phi, i, clone_iff(b->as_Phi(), loop));
     } else {
-      assert( b->is_Bool(), "" );
+      assert(b->is_Bool() || b->Opcode() == Op_Opaque4, "");
     }
   }
 
-  Node *sample_bool = phi->in(1);
-  Node *sample_cmp  = sample_bool->in(1);
+  Node* n = phi->in(1);
+  Node* sample_opaque = NULL;
+  Node *sample_bool = NULL;
+  if (n->Opcode() == Op_Opaque4) {
+    sample_opaque = n;
+    sample_bool = n->in(1);
+    assert(sample_bool->is_Bool(), "wrong type");
+  } else {
+    sample_bool = n;
+  }
+  Node *sample_cmp = sample_bool->in(1);
 
   // Make Phis to merge the Cmp's inputs.
-  PhiNode *phi1 = new PhiNode( phi->in(0), Type::TOP );
-  PhiNode *phi2 = new PhiNode( phi->in(0), Type::TOP );
-  for( i = 1; i < phi->req(); i++ ) {
-    Node *n1 = phi->in(i)->in(1)->in(1);
-    Node *n2 = phi->in(i)->in(1)->in(2);
-    phi1->set_req( i, n1 );
-    phi2->set_req( i, n2 );
-    phi1->set_type( phi1->type()->meet_speculative(n1->bottom_type()));
-    phi2->set_type( phi2->type()->meet_speculative(n2->bottom_type()));
+  PhiNode *phi1 = new PhiNode(phi->in(0), Type::TOP);
+  PhiNode *phi2 = new PhiNode(phi->in(0), Type::TOP);
+  for (i = 1; i < phi->req(); i++) {
+    Node *n1 = sample_opaque == NULL ? phi->in(i)->in(1)->in(1) : phi->in(i)->in(1)->in(1)->in(1);
+    Node *n2 = sample_opaque == NULL ? phi->in(i)->in(1)->in(2) : phi->in(i)->in(1)->in(1)->in(2);
+    phi1->set_req(i, n1);
+    phi2->set_req(i, n2);
+    phi1->set_type(phi1->type()->meet_speculative(n1->bottom_type()));
+    phi2->set_type(phi2->type()->meet_speculative(n2->bottom_type()));
   }
   // See if these Phis have been made before.
   // Register with optimizer
   Node *hit1 = _igvn.hash_find_insert(phi1);
-  if( hit1 ) {                  // Hit, toss just made Phi
+  if (hit1) {                   // Hit, toss just made Phi
     _igvn.remove_dead_node(phi1); // Remove new phi
-    assert( hit1->is_Phi(), "" );
+    assert(hit1->is_Phi(), "" );
     phi1 = (PhiNode*)hit1;      // Use existing phi
   } else {                      // Miss
     _igvn.register_new_node_with_optimizer(phi1);
   }
   Node *hit2 = _igvn.hash_find_insert(phi2);
-  if( hit2 ) {                  // Hit, toss just made Phi
+  if (hit2) {                   // Hit, toss just made Phi
     _igvn.remove_dead_node(phi2); // Remove new phi
-    assert( hit2->is_Phi(), "" );
+    assert(hit2->is_Phi(), "" );
     phi2 = (PhiNode*)hit2;      // Use existing phi
   } else {                      // Miss
     _igvn.register_new_node_with_optimizer(phi2);
@@ -1457,8 +1466,8 @@
   set_ctrl(phi2, phi->in(0));
   // Make a new Cmp
   Node *cmp = sample_cmp->clone();
-  cmp->set_req( 1, phi1 );
-  cmp->set_req( 2, phi2 );
+  cmp->set_req(1, phi1);
+  cmp->set_req(2, phi2);
   _igvn.register_new_node_with_optimizer(cmp);
   set_ctrl(cmp, phi->in(0));
 
@@ -1468,8 +1477,16 @@
   _igvn.register_new_node_with_optimizer(b);
   set_ctrl(b, phi->in(0));
 
-  assert( b->is_Bool(), "" );
-  return (BoolNode*)b;
+  if (sample_opaque != NULL) {
+    Node* opaque = sample_opaque->clone();
+    opaque->set_req(1, b);
+    _igvn.register_new_node_with_optimizer(opaque);
+    set_ctrl(opaque, phi->in(0));
+    return opaque;
+  }
+
+  assert(b->is_Bool(), "");
+  return b;
 }
 
 //------------------------------clone_bool-------------------------------------
@@ -1749,21 +1766,24 @@
         // in the loop to break the loop, then test is again outside of the
         // loop to determine which way the loop exited.
         // Loop predicate If node connects to Bool node through Opaque1 node.
-        if (use->is_If() || use->is_CMove() || C->is_predicate_opaq(use)) {
+        if (use->is_If() || use->is_CMove() || C->is_predicate_opaq(use) || use->Opcode() == Op_Opaque4) {
           // Since this code is highly unlikely, we lazily build the worklist
           // of such Nodes to go split.
-          if( !split_if_set )
+          if (!split_if_set) {
             split_if_set = new Node_List(area);
+          }
           split_if_set->push(use);
         }
-        if( use->is_Bool() ) {
-          if( !split_bool_set )
+        if (use->is_Bool()) {
+          if (!split_bool_set) {
             split_bool_set = new Node_List(area);
+          }
           split_bool_set->push(use);
         }
-        if( use->Opcode() == Op_CreateEx ) {
-          if( !split_cex_set )
+        if (use->Opcode() == Op_CreateEx) {
+          if (!split_cex_set) {
             split_cex_set = new Node_List(area);
+          }
           split_cex_set->push(use);
         }
 
@@ -1852,31 +1872,31 @@
   // takes control from one or more OLD Regions (which in turn get from NEW
   // Regions).  In any case, there will be a set of Phis for each merge point
   // from the IF up to where the original BOOL def exists the loop.
-  if( split_if_set ) {
-    while( split_if_set->size() ) {
+  if (split_if_set) {
+    while (split_if_set->size()) {
       Node *iff = split_if_set->pop();
-      if( iff->in(1)->is_Phi() ) {
-        BoolNode *b = clone_iff( iff->in(1)->as_Phi(), loop );
+      if (iff->in(1)->is_Phi()) {
+        Node *b = clone_iff(iff->in(1)->as_Phi(), loop);
         _igvn.replace_input_of(iff, 1, b);
       }
     }
   }
-  if( split_bool_set ) {
-    while( split_bool_set->size() ) {
+  if (split_bool_set) {
+    while (split_bool_set->size()) {
       Node *b = split_bool_set->pop();
       Node *phi = b->in(1);
-      assert( phi->is_Phi(), "" );
-      CmpNode *cmp = clone_bool( (PhiNode*)phi, loop );
+      assert(phi->is_Phi(), "");
+      CmpNode *cmp = clone_bool((PhiNode*)phi, loop);
       _igvn.replace_input_of(b, 1, cmp);
     }
   }
-  if( split_cex_set ) {
-    while( split_cex_set->size() ) {
+  if (split_cex_set) {
+    while (split_cex_set->size()) {
       Node *b = split_cex_set->pop();
-      assert( b->in(0)->is_Region(), "" );
-      assert( b->in(1)->is_Phi(), "" );
-      assert( b->in(0)->in(0) == b->in(1)->in(0), "" );
-      split_up( b, b->in(0), NULL );
+      assert(b->in(0)->is_Region(), "");
+      assert(b->in(1)->is_Phi(), "");
+      assert(b->in(0)->in(0) == b->in(1)->in(0), "");
+      split_up(b, b->in(0), NULL);
     }
   }
 
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/hotspot/jtreg/compiler/unsafe/TestLoopUnswitching.java	Mon Nov 27 16:05:11 2017 -0800
@@ -0,0 +1,102 @@
+/*
+ * Copyright (c) 2017, Red Hat, Inc. 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 8191887
+ * @summary loop cloning misses support for Opaque4 node
+ * @modules java.base/jdk.internal.misc:+open
+ *
+ * @run main/othervm -XX:-BackgroundCompilation TestLoopUnswitching
+ *
+ */
+
+import jdk.internal.misc.Unsafe;
+import java.lang.reflect.Field;
+import java.util.Arrays;
+
+public class TestLoopUnswitching {
+
+    static final jdk.internal.misc.Unsafe UNSAFE = Unsafe.getUnsafe();
+    static final long F_OFFSET;
+
+    static class A {
+        int f;
+        A(int f) {
+            this.f = f;
+        }
+    }
+
+    static {
+        try {
+            Field fField = A.class.getDeclaredField("f");
+            F_OFFSET = UNSAFE.objectFieldOffset(fField);
+        } catch (Exception e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    static int test1(A[] arr, boolean flag1, boolean flag2) {
+        int res = 0;
+        for (int i = 0; i < 10; i++) {
+            A a = arr[i];
+            if (flag1) { // triggers unswitching
+                res += UNSAFE.getInt(a, F_OFFSET);
+            }
+            if (flag2) {
+                // Opaque4 node here is in the loop but If is out of the loop
+                res += UNSAFE.getInt(a, F_OFFSET);
+                break;
+            }
+            res += UNSAFE.getInt(a, F_OFFSET);
+        }
+        return res;
+    }
+
+    static int test2(A[] arr, boolean flag1, boolean flag2) {
+        int res = 0;
+        for (int i = 0; i < 10; i++) {
+            A a = arr[i];
+            if (flag1) { // triggers unswitching
+                res += UNSAFE.getInt(a, F_OFFSET);
+            }
+            if (flag2) {
+                // Opaque4 node here is out of the loop but Bool is in the the loop
+                res += UNSAFE.getInt(a, F_OFFSET);
+                break;
+            }
+            res += a.f;
+        }
+        return res;
+    }
+
+    static public void main(String[] args) {
+        A[] arr = new A[1000];
+        Arrays.fill(arr, new A(0x42));
+        for (int i = 0; i < 20000; i++) {
+            test1(arr, (i%2) == 0, (i%2) == 0);
+            test2(arr, (i%2) == 0, (i%2) == 0);
+        }
+    }
+
+}