RFR(S): 8168283: adlc: fix error expanding expanded nodes.

[Lindenmaier, Goetz]

Hi,

Is this good now?  Could someone please sponsor?

Thanks!
  Goetz.

> -----Original Message-----
> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
> bounces at openjdk.java.net] On Behalf Of Lindenmaier, Goetz
> Sent: Freitag, 21. Oktober 2016 09:36
> To: Vladimir Kozlov <vladimir.kozlov at oracle.com>; hotspot-compiler-
> dev at openjdk.java.net
> Subject: RE: RFR(S): 8168283: adlc: fix error expanding expanded nodes.
> 
> Hi,
> 
> I added an assertion, generated code looks like this now:
> 
>   // Remove duplicated operands and inputs which use the same name.
>   if (num_opnds() == 4) {
>     unsigned num0 = 0;
>     unsigned num1 = opnd_array(1)->num_edges(); 	// mem
>     unsigned num2 = opnd_array(2)->num_edges(); 	// twentyfour
>     unsigned num3 = opnd_array(3)->num_edges(); 	// twentyfour
>     unsigned idx0 = oper_input_base();
>     unsigned idx1 = idx0 + num0;
>     unsigned idx2 = idx1 + num1;
>     unsigned idx3 = idx2 + num2;
>     unsigned idx4 = idx3 + num3;
>     for (int i = idx4 - 1; i >= (int)idx3; i--) {
>       del_req(i);
>     }
>     _num_opnds = 3;
>   } else {
>     assert(_num_opnds == 3, "There should be either 3 or 4 operands.");
>   }
> 
> New webrev:
>  http://cr.openjdk.java.net/~goetz/wr16/8168283-adlc_expand/webrev.02/
> 
> Best regards,
>   Goetz.
> 
> > -----Original Message-----
> > From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> > Sent: Donnerstag, 20. Oktober 2016 19:09
> > To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-compiler-
> > dev at openjdk.java.net
> > Subject: Re: RFR(S): 8168283: adlc: fix error expanding expanded nodes.
> >
> > On 10/19/16 11:16 PM, Lindenmaier, Goetz wrote:
> > > Hi Vladimir,
> > >
> > > thanks for looking at my change.
> > >
> > >> Should we use next ?
> > >> if (num_opnds() > %d) {\n", node->num_unique_opnds());
> > >
> > > The code within the if accesses the opnd_array up to index
> > 'cur_num_opnds'.
> > > Therefore num_opnds() must be at least that number.
> > > It is opnd_array(cur_num_opnds-1)->num_edges() what is crashing in
> my
> > case.
> > >
> > > It would be possible to check >=, but I don't think that makes sense.
> >
> > Yes, num_opnds() >= cur_num_opnds does not make sense. That is why I
> > thought to use num_unique_opnds.
> >
> > > But I could assert that num_opnds() is either cur_num_opnds or
> > num_unique_opnds().
> >
> > Would be nice.
> >
> > thanks,
> > Vladimir
> >
> > >
> > > Best regards,
> > >   Goetz.
> > >
> > >
> > >> -----Original Message-----
> > >> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
> > >> bounces at openjdk.java.net] On Behalf Of Vladimir Kozlov
> > >> Sent: Donnerstag, 20. Oktober 2016 00:26
> > >> To: hotspot-compiler-dev at openjdk.java.net
> > >> Subject: Re: RFR(S): 8168283: adlc: fix error expanding expanded nodes.
> > >>
> > >> Should we use next ?
> > >>
> > >> if (num_opnds() > %d) {\n", node->num_unique_opnds());
> > >>
> > >> instead of
> > >>
> > >> +      fprintf(fp, "  // Remove duplicated operands and inputs which use
> > >> the same name.\n");
> > >> +      fprintf(fp, "  if (num_opnds() == %d) {\n", cur_num_opnds);
> > >>
> > >> Otherwise seems good.
> > >>
> > >> Thanks,
> > >> Vladimir
> > >>
> > >> On 10/19/16 7:44 AM, Lindenmaier, Goetz wrote:
> > >>> Hi,
> > >>>
> > >>> This fixes a small error in the expand methods usually generated from
> > >> expand rules by adlc.
> > >>>
> > >>> Actually all nodes get an expand method generated, as it's used to
> finish
> > >> construction of nodes after the matcher generated them. E.g., temp
> > nodes
> > >> are added in the expand.
> > >>>
> > >>> The error occurs for a 'real' expand generating several sub nodes. The
> > 'real'
> > >> expand rule calls expand of the newly generated nodes, let's call it sub-
> > >> expand.
> > >>>
> > >>> Unfortunately the matcher sometimes generates superfluous
> operands
> > >> that are removed in the expand methods. The 'real' expand generating
> > sub-
> > >> nodes does not generate these superfluous operands, therefore
> running
> > the
> > >> sub-expand crashes when it tries to remove these.
> > >>>
> > >>> This change adds a simple check in the sub-expand to avoid this crash.
> > Also
> > >> it adds setting the correct number of operands in the expand that
> > generates
> > >> the sub node.
> > >>>
> > >>> An example of an adl instruct declaration and the generated code is
> > >> attached to the bug. We see this in a rule for the s390 port.
> > >>>
> > >>> http://cr.openjdk.java.net/~goetz/wr16/8168283-
> > >> adlc_expand/webrev.01/
> > >>> https://bugs.openjdk.java.net/browse/JDK-8168283
> > >>>
> > >>> Please review this change. I please need a sponsor.
> > >>>
> > >>> Best regards,
> > >>>   Goetz.
> > >>>