8314997: Missing optimization opportunities due to missing try_clean_mem_phi() calls

Reviewed-by: roland, kvn, thartmann

Author: chhagedorn

src/hotspot/share/opto/cfgnode.cpp

@@ -447,7 +447,7 @@ void RegionNode::verify_can_be_irreducible_entry() const {
 }
 #endif //ASSERT
 
-bool RegionNode::try_clean_mem_phi(PhaseGVN *phase) {
+void RegionNode::try_clean_mem_phis(PhaseIterGVN* igvn) {
   // Incremental inlining + PhaseStringOpts sometimes produce:
   //
   // cmpP with 1 top input
@@ -464,32 +464,60 @@ bool RegionNode::try_clean_mem_phi(PhaseGVN *phase) {
   // replaced by If's control input but because there's still a Phi,
   // the Region stays in the graph. The top input from the cmpP is
   // propagated forward and a subgraph that is useful goes away. The
-  // code below replaces the Phi with the MergeMem so that the Region
-  // is simplified.
-
-  PhiNode* phi = has_unique_phi();
-  if (phi && phi->type() == Type::MEMORY && req() == 3 && phi->is_diamond_phi(true)) {
-    MergeMemNode* m = nullptr;
-    assert(phi->req() == 3, "same as region");
-    for (uint i = 1; i < 3; ++i) {
-      Node *mem = phi->in(i);
-      if (mem && mem->is_MergeMem() && in(i)->outcnt() == 1) {
-        // Nothing is control-dependent on path #i except the region itself.
-        m = mem->as_MergeMem();
-        uint j = 3 - i;
-        Node* other = phi->in(j);
-        if (other && other == m->base_memory()) {
-          // m is a successor memory to other, and is not pinned inside the diamond, so push it out.
-          // This will allow the diamond to collapse completely.
-          phase->is_IterGVN()->replace_node(phi, m);
-          return true;
-        }
-      }
+  // code in PhiNode::try_clean_memory_phi() replaces the Phi with the
+  // MergeMem in order to remove the Region if its last phi dies.
+
+  if (!is_diamond()) {
+    return;
+  }
+
+  for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) {
+    Node* phi = fast_out(i);
+    if (phi->is_Phi() && phi->as_Phi()->try_clean_memory_phi(igvn)) {
+      --i;
+      --imax;
     }
   }
-  return false;
 }
 
+// Does this region merge a simple diamond formed by a proper IfNode?
+//
+//              Cmp
+//              /
+//     ctrl   Bool
+//       \    /
+//       IfNode
+//      /      \
+//  IfFalse   IfTrue
+//      \      /
+//       Region
+bool RegionNode::is_diamond() const {
+  if (req() != 3) {
+    return false;
+  }
+
+  Node* left_path = in(1);
+  Node* right_path = in(2);
+  if (left_path == nullptr || right_path == nullptr) {
+    return false;
+  }
+  Node* diamond_if = left_path->in(0);
+  if (diamond_if == nullptr || !diamond_if->is_If() || diamond_if != right_path->in(0)) {
+    // Not an IfNode merging a diamond or TOP.
+    return false;
+  }
+
+  // Check for a proper bool/cmp
+  const Node* bol = diamond_if->in(1);
+  if (!bol->is_Bool()) {
+    return false;
+  }
+  const Node* cmp = bol->in(1);
+  if (!cmp->is_Cmp()) {
+    return false;
+  }
+  return true;
+}
 //------------------------------Ideal------------------------------------------
 // Return a node which is more "ideal" than the current node.  Must preserve
 // the CFG, but we can still strip out dead paths.
@@ -501,10 +529,8 @@ Node *RegionNode::Ideal(PhaseGVN *phase, bool can_reshape) {
   // arm of the same IF.  If found, then the control-flow split is useless.
   bool has_phis = false;
   if (can_reshape) {            // Need DU info to check for Phi users
+    try_clean_mem_phis(phase->is_IterGVN());
     has_phis = (has_phi() != nullptr);       // Cache result
-    if (has_phis && try_clean_mem_phi(phase)) {
-      has_phis = false;
-    }
 
     if (!has_phis) {            // No Phi users?  Nothing merging?
       for (uint i = 1; i < req()-1; i++) {
@@ -1327,42 +1353,60 @@ const Type* PhiNode::Value(PhaseGVN* phase) const {
   return ft;
 }
 
-
-//------------------------------is_diamond_phi---------------------------------
 // Does this Phi represent a simple well-shaped diamond merge?  Return the
 // index of the true path or 0 otherwise.
-// If check_control_only is true, do not inspect the If node at the
-// top, and return -1 (not an edge number) on success.
-int PhiNode::is_diamond_phi(bool check_control_only) const {
-  // Check for a 2-path merge
-  Node *region = in(0);
-  if( !region ) return 0;
-  if( region->req() != 3 ) return 0;
-  if(         req() != 3 ) return 0;
-  // Check that both paths come from the same If
-  Node *ifp1 = region->in(1);
-  Node *ifp2 = region->in(2);
-  if( !ifp1 || !ifp2 ) return 0;
-  Node *iff = ifp1->in(0);
-  if( !iff || !iff->is_If() ) return 0;
-  if( iff != ifp2->in(0) ) return 0;
-  if (check_control_only)  return -1;
-  // Check for a proper bool/cmp
-  const Node *b = iff->in(1);
-  if( !b->is_Bool() ) return 0;
-  const Node *cmp = b->in(1);
-  if( !cmp->is_Cmp() ) return 0;
-
-  // Check for branching opposite expected
-  if( ifp2->Opcode() == Op_IfTrue ) {
-    assert( ifp1->Opcode() == Op_IfFalse, "" );
-    return 2;
-  } else {
-    assert( ifp1->Opcode() == Op_IfTrue, "" );
+int PhiNode::is_diamond_phi() const {
+  Node* region = in(0);
+  assert(region != nullptr && region->is_Region(), "phi must have region");
+  if (!region->as_Region()->is_diamond()) {
+    return 0;
+  }
+
+  if (region->in(1)->is_IfTrue()) {
+    assert(region->in(2)->is_IfFalse(), "bad If");
     return 1;
+  } else {
+    // Flipped projections.
+    assert(region->in(2)->is_IfTrue(), "bad If");
+    return 2;
   }
 }
 
+// Do the following transformation if we find the corresponding graph shape, remove the involved memory phi and return
+// true. Otherwise, return false if the transformation cannot be applied.
+//
+//           If                                     If
+//          /  \                                   /  \
+//    IfFalse  IfTrue  /- Some Node          IfFalse  IfTrue
+//          \  /      /    /                       \  /        Some Node
+//         Region    / /-MergeMem     ===>        Region          |
+//          /   \---Phi                             |          MergeMem
+// [other phis]      \                        [other phis]        |
+//                   use                                         use
+bool PhiNode::try_clean_memory_phi(PhaseIterGVN* igvn) {
+  if (_type != Type::MEMORY) {
+    return false;
+  }
+  assert(is_diamond_phi() > 0, "sanity");
+  assert(req() == 3, "same as region");
+  const Node* region = in(0);
+  for (uint i = 1; i < 3; i++) {
+    Node* phi_input = in(i);
+    if (phi_input != nullptr && phi_input->is_MergeMem() && region->in(i)->outcnt() == 1) {
+      // Nothing is control-dependent on path #i except the region itself.
+      MergeMemNode* merge_mem = phi_input->as_MergeMem();
+      uint j = 3 - i;
+      Node* other_phi_input = in(j);
+      if (other_phi_input != nullptr && other_phi_input == merge_mem->base_memory()) {
+        // merge_mem is a successor memory to other_phi_input, and is not pinned inside the diamond, so push it out.
+        // This will allow the diamond to collapse completely if there are no other phis left.
+        igvn->replace_node(this, merge_mem);
+        return true;
+      }
+    }
+  }
+  return false;
+}
 //----------------------------check_cmove_id-----------------------------------
 // Check for CMove'ing a constant after comparing against the constant.
 // Happens all the time now, since if we compare equality vs a constant in

src/hotspot/share/opto/cfgnode.hpp

@@ -132,7 +132,8 @@ class RegionNode : public Node {
   virtual Node* Ideal(PhaseGVN* phase, bool can_reshape);
   void remove_unreachable_subgraph(PhaseIterGVN* igvn);
   virtual const RegMask &out_RegMask() const;
-  bool try_clean_mem_phi(PhaseGVN* phase);
+  bool is_diamond() const;
+  void try_clean_mem_phis(PhaseIterGVN* phase);
   bool optimize_trichotomy(PhaseIterGVN* igvn);
   NOT_PRODUCT(virtual void dump_spec(outputStream* st) const;)
 };
@@ -233,7 +234,8 @@ class PhiNode : public TypeNode {
   LoopSafety simple_data_loop_check(Node *in) const;
   // Is it unsafe data loop? It becomes a dead loop if this phi node removed.
   bool is_unsafe_data_reference(Node *in) const;
-  int  is_diamond_phi(bool check_control_only = false) const;
+  int is_diamond_phi() const;
+  bool try_clean_memory_phi(PhaseIterGVN* igvn);
   virtual int Opcode() const;
   virtual bool pinned() const { return in(0) != 0; }
   virtual const TypePtr *adr_type() const { verify_adr_type(true); return _adr_type; }

test/hotspot/jtreg/compiler/c2/irTests/igvn/TestCleanMemPhi.java

@@ -0,0 +1,126 @@
+/*
+ * Copyright (c) 2023, 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.
+ */
+package compiler.c2.irTests.igvn;
+
+import compiler.lib.ir_framework.*;
+import jdk.test.lib.Asserts;
+
+/*
+ * @test
+ * @bug 8314997
+ * @requires vm.debug == true & vm.compiler2.enabled
+ * @summary Test that diamond if-region is removed due to calling try_clean_mem_phi().
+ * @library /test/lib /
+ * @run driver compiler.c2.irTests.igvn.TestCleanMemPhi
+ */
+public class TestCleanMemPhi {
+    static boolean flag, flag2;
+    static int iFld;
+
+    public static void main(String[] args) {
+        TestFramework testFramework = new TestFramework();
+        testFramework.setDefaultWarmup(0);
+        testFramework.addFlags("-XX:+AlwaysIncrementalInline", "-XX:-PartialPeelLoop", "-XX:-LoopUnswitching");
+        testFramework.addScenarios(new Scenario(1, "-XX:-StressIGVN"),
+                                   new Scenario(2, "-XX:+StressIGVN"));
+        testFramework.start();
+    }
+
+    static class A {
+        int i;
+    }
+
+
+    static A a1 = new A();
+    static A a2 = new A();
+
+
+    @Test
+    @IR(counts = {IRNode.COUNTED_LOOP, "> 0"})
+    static void testCountedLoop() {
+        int zero = 34;
+
+        int limit = 2;
+        for (; limit < 4; limit *= 2) ;
+        for (int i = 2; i < limit; i++) {
+            zero = 0;
+        }
+
+        // Loop is not converted to a counted loop because a diamond is not removed due to missing to call
+        // try_clean_mem_phi() again on the diamond region.
+        int i = 0;
+        do {
+            iFld = 34;
+            if (flag2) {
+                iFld++;
+            }
+
+            int z = 34;
+            if (flag) {
+                lateInline(); // Inlined late -> leaves a MergeMem
+                if (zero == 34) { // False but only cleaned up after CCP
+                    iFld = 38;
+                }
+                z = 32; // Ensures to get a diamond If-Region
+            }
+            // Region merging a proper diamond after CCP with a memory phi merging loop phi and the MergeMem from lateInline().
+            // Region is not added to the IGVN worklist anymore once the second phi dies.
+            i++;
+        } while (zero == 34 || (i < 1000 && a1 == a2)); // Could be converted to a counted loop after the diamond is removed after CCP.
+    }
+
+    @Test
+    @IR(failOn = IRNode.LOOP)
+    static void testRemoveLoop() {
+        int zero = 34;
+
+        int limit = 2;
+        for (; limit < 4; limit *= 2) ;
+        for (int i = 2; i < limit; i++) {
+            zero = 0;
+        }
+
+        // Loop is not converted to a counted loop and thus cannot be removed as empty loop because a diamond is not
+        // removed due to missing to call try_clean_mem_phi() again on the diamond region.
+        int i = 0;
+        do {
+            iFld = 34;
+
+            int z = 34;
+            if (flag) {
+                lateInline(); // Inlined late -> leaves a MergeMem
+                if (zero == 34) { // False but only cleaned up after CCP
+                    iFld = 38;
+                }
+                z = 32; // Ensures to get a diamond If-Region
+            }
+            // Region merging a proper diamond after CCP with a memory phi merging loop phi and the MergeMem from lateInline().
+            // Region is not added to the IGVN worklist anymore once the second phi dies.
+
+            i++;
+        } while (zero == 34 || (i < 21 && a1 == a2));
+    }
+
+    static void lateInline() {
+    }
+}