8287217: C2: PhaseCCP: remove not visited nodes, prevent type inconsistency

Reviewed-by: kvn
Backport-of: 379f3094db0b8afe90ed6b7a341164222744085f

Author: TobiHartmann

src/hotspot/share/opto/compile.cpp

@@ -402,7 +402,7 @@ void Compile::remove_useless_node(Node* dead) {
 }
 
 // Disconnect all useless nodes by disconnecting those at the boundary.
-void Compile::remove_useless_nodes(Unique_Node_List &useful) {
+void Compile::disconnect_useless_nodes(Unique_Node_List &useful, Unique_Node_List* worklist) {
   uint next = 0;
   while (next < useful.size()) {
     Node *n = useful.at(next++);
@@ -425,7 +425,7 @@ void Compile::remove_useless_nodes(Unique_Node_List &useful) {
       }
     }
     if (n->outcnt() == 1 && n->has_special_unique_user()) {
-      record_for_igvn(n->unique_out());
+      worklist->push(n->unique_out());
     }
   }
 
@@ -435,6 +435,11 @@ void Compile::remove_useless_nodes(Unique_Node_List &useful) {
   remove_useless_nodes(_expensive_nodes,    useful); // remove useless expensive nodes
   remove_useless_nodes(_for_post_loop_igvn, useful); // remove useless node recorded for post loop opts IGVN pass
   remove_useless_coarsened_locks(useful);            // remove useless coarsened locks nodes
+#ifdef ASSERT
+  if (_modified_nodes != NULL) {
+    _modified_nodes->remove_useless_nodes(useful.member_set());
+  }
+#endif
 
   BarrierSetC2* bs = BarrierSet::barrier_set()->barrier_set_c2();
   bs->eliminate_useless_gc_barriers(useful, this);

src/hotspot/share/opto/compile.hpp

@@ -945,7 +945,7 @@ class Compile : public Phase {
 
   void              identify_useful_nodes(Unique_Node_List &useful);
   void              update_dead_node_list(Unique_Node_List &useful);
-  void              remove_useless_nodes (Unique_Node_List &useful);
+  void              disconnect_useless_nodes(Unique_Node_List &useful, Unique_Node_List* worklist);
 
   void              remove_useless_node(Node* dead);
 

src/hotspot/share/opto/phaseX.cpp

@@ -423,7 +423,7 @@ PhaseRemoveUseless::PhaseRemoveUseless(PhaseGVN* gvn, Unique_Node_List* worklist
   worklist->remove_useless_nodes(_useful.member_set());
 
   // Disconnect 'useless' nodes that are adjacent to useful nodes
-  C->remove_useless_nodes(_useful);
+  C->disconnect_useless_nodes(_useful, worklist);
 }
 
 //=============================================================================
@@ -1764,6 +1764,9 @@ void PhaseCCP::analyze() {
   Unique_Node_List worklist;
   worklist.push(C->root());
 
+  assert(_root_and_safepoints.size() == 0, "must be empty (unused)");
+  _root_and_safepoints.push(C->root());
+
   // Pull from worklist; compute new value; push changes out.
   // This loop is the meat of CCP.
   while( worklist.size() ) {
@@ -1774,8 +1777,9 @@ void PhaseCCP::analyze() {
       n = worklist.pop();
     }
     if (n->is_SafePoint()) {
-      // Keep track of SafePoint nodes for PhaseCCP::transform()
-      _safepoints.push(n);
+      // Make sure safepoints are processed by PhaseCCP::transform even if they are
+      // not reachable from the bottom. Otherwise, infinite loops would be removed.
+      _root_and_safepoints.push(n);
     }
     const Type *t = n->Value(this);
     if (t != type(n)) {
@@ -1904,32 +1908,34 @@ Node *PhaseCCP::transform( Node *n ) {
   Node *new_node = _nodes[n->_idx]; // Check for transformed node
   if( new_node != NULL )
     return new_node;                // Been there, done that, return old answer
-  new_node = transform_once(n);     // Check for constant
-  _nodes.map( n->_idx, new_node );  // Flag as having been cloned
 
-  // Allocate stack of size _nodes.Size()/2 to avoid frequent realloc
-  GrowableArray <Node *> trstack(C->live_nodes() >> 1);
+  assert(n->is_Root(), "traversal must start at root");
+  assert(_root_and_safepoints.member(n), "root (n) must be in list");
 
-  trstack.push(new_node);           // Process children of cloned node
+  // Allocate stack of size _nodes.Size()/2 to avoid frequent realloc
+  GrowableArray <Node *> transform_stack(C->live_nodes() >> 1);
+  Unique_Node_List useful; // track all visited nodes, so that we can remove the complement
 
+  // Initialize the traversal.
   // This CCP pass may prove that no exit test for a loop ever succeeds (i.e. the loop is infinite). In that case,
   // the logic below doesn't follow any path from Root to the loop body: there's at least one such path but it's proven
   // never taken (its type is TOP). As a consequence the node on the exit path that's input to Root (let's call it n) is
   // replaced by the top node and the inputs of that node n are not enqueued for further processing. If CCP only works
   // through the graph from Root, this causes the loop body to never be processed here even when it's not dead (that
   // is reachable from Root following its uses). To prevent that issue, transform() starts walking the graph from Root
   // and all safepoints.
-  for (uint i = 0; i < _safepoints.size(); ++i) {
-    Node* nn = _safepoints.at(i);
+  for (uint i = 0; i < _root_and_safepoints.size(); ++i) {
+    Node* nn = _root_and_safepoints.at(i);
     Node* new_node = _nodes[nn->_idx];
     assert(new_node == NULL, "");
-    new_node = transform_once(nn);
-    _nodes.map(nn->_idx, new_node);
-    trstack.push(new_node);
+    new_node = transform_once(nn);  // Check for constant
+    _nodes.map(nn->_idx, new_node); // Flag as having been cloned
+    transform_stack.push(new_node); // Process children of cloned node
+    useful.push(new_node);
   }
 
-  while ( trstack.is_nonempty() ) {
-    Node *clone = trstack.pop();
+  while (transform_stack.is_nonempty()) {
+    Node* clone = transform_stack.pop();
     uint cnt = clone->req();
     for( uint i = 0; i < cnt; i++ ) {          // For all inputs do
       Node *input = clone->in(i);
@@ -1938,13 +1944,33 @@ Node *PhaseCCP::transform( Node *n ) {
         if( new_input == NULL ) {
           new_input = transform_once(input);   // Check for constant
           _nodes.map( input->_idx, new_input );// Flag as having been cloned
-          trstack.push(new_input);
+          transform_stack.push(new_input);     // Process children of cloned node
+          useful.push(new_input);
         }
         assert( new_input == clone->in(i), "insanity check");
       }
     }
   }
-  return new_node;
+
+  // The above transformation might lead to subgraphs becoming unreachable from the
+  // bottom while still being reachable from the top. As a result, nodes in that
+  // subgraph are not transformed and their bottom types are not updated, leading to
+  // an inconsistency between bottom_type() and type(). In rare cases, LoadNodes in
+  // such a subgraph, might be re-enqueued for IGVN indefinitely by MemNode::Ideal_common
+  // because their address type is inconsistent. Therefore, we aggressively remove
+  // all useless nodes here even before PhaseIdealLoop::build_loop_late gets a chance
+  // to remove them anyway.
+  if (C->cached_top_node()) {
+    useful.push(C->cached_top_node());
+  }
+  C->update_dead_node_list(useful);
+  remove_useless_nodes(useful.member_set());
+  _worklist.remove_useless_nodes(useful.member_set());
+  C->disconnect_useless_nodes(useful, &_worklist);
+
+  Node* new_root = _nodes[n->_idx];
+  assert(new_root->is_Root(), "transformed root node must be a root node");
+  return new_root;
 }
 
 

src/hotspot/share/opto/phaseX.hpp

@@ -565,7 +565,7 @@ class PhaseIterGVN : public PhaseGVN {
 // Phase for performing global Conditional Constant Propagation.
 // Should be replaced with combined CCP & GVN someday.
 class PhaseCCP : public PhaseIterGVN {
-  Unique_Node_List _safepoints;
+  Unique_Node_List _root_and_safepoints;
   // Non-recursive.  Use analysis to transform single Node.
   virtual Node *transform_once( Node *n );
 

test/hotspot/jtreg/compiler/ccp/TestRemoveUnreachableCCP.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2022, 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 8287217
+ * @summary CCP must remove nodes that are not traversed, else their type can be inconsistent
+ * @run main/othervm -Xcomp -XX:CompileCommand=compileonly,TestRemoveUnreachableCCP::test
+ *                   TestRemoveUnreachableCCP
+ */
+
+public class TestRemoveUnreachableCCP {
+
+    static void test() {
+        Byte x = 1;
+        for (int i = 0; i < 10000; i++) {
+            if ((i & 1) == 0) {
+                x = (byte)x;
+            }
+        }
+    }
+
+    public static void main(String[] strArr) {
+        for (int i = 0; i < 11; i++) {
+            test();
+        }
+    }
+}