RFR(S): 8250609: C2 crash in IfNode::fold_compares

[Christian Hagedorn]

Hi Felix

Looks good to me.

Just some minor comments (no new webrev required):
- L996: The asterisk should be at the type (Node*)
- L997/1003: You could remove one extra whitespace before the comment starts

Best regards,
Christian

On 29.07.20 05:20, Yangfei (Felix) wrote:
> Hi,
> 
>      Thanks for reviewing this.
>      Committed to jdk/submit repo and test result received looks good.
>      Will do the push.
> 
> Thanks,
> Felix
> 
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Wednesday, July 29, 2020 1:28 AM
>> To: Yangfei (Felix) <felix.yang at huawei.com>; hotspot-compiler-
>> dev at openjdk.java.net
>> Subject: Re: RFR(S): 8250609: C2 crash in IfNode::fold_compares
>>
>> Yes, this looks good.
>>
>> Thanks,
>> Vladimir K
>>
>> On 7/28/20 5:10 AM, Yangfei (Felix) wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>> Sent: Tuesday, July 28, 2020 3:15 AM
>>>> To: Yangfei (Felix) <felix.yang at huawei.com>; hotspot-compiler-
>>>> dev at openjdk.java.net
>>>> Subject: Re: RFR(S): 8250609: C2 crash in IfNode::fold_compares
>>>>
>>>> It happens because 'lo' is new node created just now and have no uses
>> yet.
>>>> For such new nodes we usually add dummy use to avoid removal from
>> graph:
>>>>
>>>> http://hg.openjdk.java.net/jdk/jdk/file/c379dc750a02/src/hotspot/shar
>>>> e/op
>>>> to/convertnode.cpp#l403
>>>
>>> Thanks for the suggestions.  Yes, that will also fix the issue.
>>> New webrev: http://cr.openjdk.java.net/~fyang/8250609/webrev.01/
>>> Performed the same tests as before.  Does it look better?
>>>
>>> Felix
>>>
>>>> On 7/27/20 5:27 AM, Yangfei (Felix) wrote:
>>>>> Hi,
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8250609
>>>>> Webrev: http://cr.openjdk.java.net/~fyang/8250609/webrev.00/
>>>>>
>>>>> In IfNode::fold_compares_helper, C2 tries to fold 2 CmpI into a
>>>>> single
>>>> CmpU.
>>>>> At the crash site in IfNode::fold_compares_helper:
>>>>>     995   if (lo && hi) {
>>>>>     996     // Merge the two compares into a single unsigned compare by
>>>> building (CmpU (n - lo) (hi - lo))
>>>>>     997     Node* adjusted_val = igvn->transform(new SubINode(n,  lo));
>>>>>     998     if (adjusted_lim == NULL) {
>>>>>     999       adjusted_lim = igvn->transform(new SubINode(hi, lo));
>>>>> 1000     }
>>>>>
>>>>> At line 997, we have:
>>>>> (gdb) p lo->dump()
>>>>>     641    AddI    === _  513  92  [[]]
>>>>> $1 = void
>>>>>
>>>>> After the transformation at line 997, we have
>>>>> (gdb) p lo->dump()
>>>>>     641    AddI    === _ _ _  [[]]   [34200641]
>>>>> $3 = void
>>>>>
>>>>> Then node 641 was used at line 999, which triggers the crash.
>>>>> Patch fixes the issue by delaying transformation in
>>>>> IfNode::fold_compares
>>>> temporarily.
>>>>> Tier1-3 tested on aarch64-linux-gnu & x86_64-linux-gnu.
>>>>> Newly added test fail without the patch and pass otherwise.
>>>>> Suggestions?
>>>>>
>>>>> Thanks,
>>>>> Felix
>>>>>