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

[Tobias Hartmann]

Hi Felix,

that looks good to me. I'll run some perf and correctness testing and report back once it finished.

Best regards,
Tobias

On 30.06.20 14:35, Yangfei (Felix) wrote:
> 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.
>>
>