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

[Yangfei (Felix)]

Hi again,

    Updated webrev: http://cr.openjdk.java.net/~fyang/8243670/webrev.03/ 
    Tier1-3 tested with fastdebug build both on aarch64-linux-gnu & x86_64-linux-gnu. 
    Newly added test case fail without the fix and pass otherwise. 
    Please take another look. 

Thanks,
Felix

> -----Original Message-----
> From: Yangfei (Felix)
> Sent: Tuesday, June 30, 2020 4:00 PM
> To: 'Roland Westrelin' <rwestrel at redhat.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,
> 
> > -----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.
>