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

[Yangfei (Felix)]

Hi Christian,

Thanks for the careful reviewing :-)
That will be easy to fix and I will modify when I push.

Felix

> -----Original Message-----
> From: Christian Hagedorn [mailto:christian.hagedorn at oracle.com]
> Sent: Wednesday, July 29, 2020 3:03 PM
> To: Yangfei (Felix) <felix.yang at huawei.com>; Vladimir Kozlov
> <vladimir.kozlov at oracle.com>; hotspot-compiler-dev at openjdk.java.net
> Subject: Re: RFR(S): 8250609: C2 crash in IfNode::fold_compares
> 
> 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/sh
> >>>> ar
> >>>> 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
> >>>>>