RFR(S): 8243670: Unexpected test result caused by C2 MergeMemNode::Ideal

[Yangfei (Felix)]

Hi Roland,

> -----Original Message-----
> From: Roland Westrelin [mailto:rwestrel at redhat.com]
> Sent: Friday, June 19, 2020 10:28 PM
> To: Yangfei (Felix) <felix.yang at huawei.com>; Tobias Hartmann
> <tobias.hartmann at oracle.com>; hotspot-compiler-dev at openjdk.java.net
> Subject: RE: RFR(S): 8243670: Unexpected test result caused by C2
> MergeMemNode::Ideal
> 
> 
> Hi Felix,
> 
> > So I guess it might be safer to go with the initially proposed patch.
> 
> What about a PhiNode::Identity transformation that, for a memory phi, looks
> for phis with identical inputs and if it finds one that has type
> TypePtr::BOTTOM, replaces the current phi with the bottom phi? Wouldn't
> that result in both the LoadI and the MergeMem with the same memory
> input which would fix the problem?

That sounds promising.  I tried the following patch and looks like it works as expected.

diff -r 2342d5af52b7 src/hotspot/share/opto/cfgnode.cpp
--- a/src/hotspot/share/opto/cfgnode.cpp        Mon Jun 22 08:09:23 2020 +0200
+++ b/src/hotspot/share/opto/cfgnode.cpp        Mon Jun 22 15:33:06 2020 +0800
@@ -1335,6 +1335,43 @@
     if (id != NULL)  return id;
   }

+  // replace equivalent phis
+  if (type()->has_memory()) {
+    for (uint i = 0; i < outcnt(); i++) {
+      Node* mmem = raw_out(i);
+      if (!mmem->is_MergeMem()) {
+        continue;
+      }
+
+      uint phi_len = req();
+      Node* phi_reg = region();
+      // Look at each slice looking for phis with identical inputs.
+      // If we find one that has type TypePtr::BOTTOM, replace the
+      // current phi with the bottom phi.
+      for (uint j = Compile::AliasIdxBot; j < mmem->req(); j++) {
+        Node* node = mmem->in(j);
+        if (node == this || !node->is_Phi()) {
+          continue;
+        }
+        if (node->req() == phi_len && node->in(0) == phi_reg) {
+          PhiNode* phi_mem = node->as_Phi();
+          if (phi_mem->adr_type() != TypePtr::BOTTOM) {
+            continue;
+          }
+          for (uint k = 1; k < phi_len; k++) {
+            if (in(k) != phi_mem->in(k)) {
+              phi_mem = NULL;
+              break;
+            }
+          }
+          if (phi_mem != NULL) {
+            return phi_mem;
+          }
+        }
+      }
+    }
+  }
+
   return this;                     // No identity
 }

With this patch, in MergeMemNode::Ideal we have:
(gdb) p this->dump()
 556    MergeMem        === _  1  660  660  1  660  [[ 559 ]]  { - - - }  Memory: @BotPTR *+bot, idx=Bot; !orig=151 !jvms: TestReplaceEquivPhis::test @ bci:25

Here, node 655 & 657 are both replaced by node 660 and this is reflected the inputs of the MergeMem node.

After MergeMemNode::Ideal, we have:
(gdb) p this->dump()
 556    MergeMem        === _  1  660  1  1  1  [[ 559 ]]  { - - - }  Memory: @BotPTR *+bot, idx=Bot; !orig=151 !jvms: TestReplaceEquivPhis::test @ bci:25

(gdb) p this->find(660)->dump()
 660    Phi     ===  679  752  583  [[ 556  533  516 ]]  #memory  Memory: @BotPTR *+bot, idx=Bot; !orig=[655]

(gdb) p this->find(516)->dump()
 516    LoadI   === _  660  423  [[ 680  799  798  797  796 ]]  @TestReplaceEquivPhis+12 *, name=iFld, idx=5; #int !orig=177 !jvms: TestReplaceEquivPhis::test @ bci:34

Tiered 1-3 tested on x86_64-linux-gnu.  Does it look good?

Thanks,
Felix