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

[Yangfei (Felix)]

Hi Roland,

  Thanks for the suggestions.

> -----Original Message-----
> From: Roland Westrelin [mailto:rwestrel at redhat.com]
> Sent: Monday, June 22, 2020 4:12 PM
> To: Yangfei (Felix) <felix.yang at huawei.com>; Tobias Hartmann
> <tobias.hartmann at oracle.com>; hotspot-compiler-dev at openjdk.java.net
> Cc: guoge (A) <guoge1 at huawei.com>; zhouyong (V)
> <zhouyong44 at huawei.com>
> Subject: RE: RFR(S): 8243670: Unexpected test result caused by C2
> MergeMemNode::Ideal
> 
> 
> Hi Felix,
> 
> See comments below
> 
> > 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()) {
> 
> It's likely unsafe to perform that transformation at GVN time so I think it
> needs to be performed only if phase->is_IterGVN() != NULL.

OK.

> > +    for (uint i = 0; i < outcnt(); i++) {
> > +      Node* mmem = raw_out(i);
> 
> The usual pattern is to use one of the iterators (DUIterator_Fast in this case).

OK.

> > +      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
> >  }
> 
> I would make this fully generic:
> 
> if (phase->is_IterGVN() && type() == Type::MEMORY && adr_type() !=
> TypePtr::BOTTOM) {
>   Node* phi_reg = region();
>   for (DUIterator_Fast imax, i = phi_reg->fast_outs(imax); i < imax; i++) {
>     Node* u = phi_reg->fast_out(i);
>     if (u->is_Phi() && u->in(0) == phi_reg && type() == Type::MEMORY && u-
> >adr_type() == TypePtr::BOTTOM) {
>       return u;
>     }
>   }
> }

That looks much better.  I modified accordingly. 
Does the updated patch look better? 
Tier1-3 tested on x86_64-linux-gnu.  Will propose a webrev for that. 

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 18:45:43 2020 +0800
@@ -1335,6 +1335,28 @@
     if (id != NULL)  return id;
   }

+  // Looking for phis with identical inputs.  If we find one that has
+  // type TypePtr::BOTTOM, replace the current phi with the bottom phi.
+  if (phase->is_IterGVN() && type() == Type::MEMORY && adr_type() != TypePtr::BOTTOM) {
+    uint phi_len = req();
+    Node* phi_reg = region();
+    for (DUIterator_Fast imax, i = phi_reg->fast_outs(imax); i < imax; i++) {
+      Node* u = phi_reg->fast_out(i);
+      if (u->is_Phi() && u->as_Phi()->type() == Type::MEMORY &&
+          u->adr_type() == TypePtr::BOTTOM && u->in(0) == phi_reg && u->req() == phi_len) {
+        for (uint j = 1; j < phi_len; j++) {
+          if (in(j) != u->in(j)) {
+            u = NULL;
+            break;
+          }
+        }
+        if (u != NULL) {
+          return u;
+        }
+      }
+    }
+  }
+
   return this;                     // No identity
 }

Felix