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

[Yangfei (Felix)]

Hi,

> -----Original Message-----
> From: Roland Westrelin [mailto:rwestrel at redhat.com]
> Sent: Tuesday, June 30, 2020 3:52 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
> 
> 
> > I added some extra code to check if we still have a chance to replace
> > equivalent phis in MergeMemNode::Ideal.
> 
> Is this a reliable way of finding a missed transformation opportunity?
> Could it be that MergeMemNode::Ideal() runs before PhiNode::Identity()
> has had a chance to run? that is the merge mem is first in the IGVN queue

Yes, I have just confirmed that.
The follow logic in MergeMemNode::Ideal() could catch phis with the same input before PhiNode::Identity() had a change to do.

4608     // replace equivalent phis (unfortunately, they do not GVN together)
4609     if (new_mem != NULL && new_mem != new_base &&
4610         new_mem->req() == phi_len && new_mem->in(0) == phi_reg) {
4611       if (new_mem->is_Phi()) {
4612         PhiNode* phi_mem = new_mem->as_Phi();
4613         for (uint i = 1; i < phi_len; i++) {
4614           if (phi_base->in(i) != phi_mem->in(i)) {
4615             phi_mem = NULL;
4616             break;
4617           }
4618         }
4619         if (phi_mem != NULL) {
4620           // equivalent phi nodes; revert to the def
4621           new_mem = new_base;
4622         }
4623       }
4624     }

So I think I will propose a new patch with this logic removed.  It should be useless with our fix in PhiNode::Identity().  

> and the Phi is next so the merge mem would first fail to optimize, then the
> Phi would optimize, cause the merge mem to be enqueued again and this
> time be properly optimized.