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

[Lindenmaier, Goetz]

Hi Vladimir, 

great, thank you!

Best regards,
  Goetz.

> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Freitag, 28. Oktober 2016 10:19
> 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.
> 
> Yes, it is good. I submitted testing.
> 
> Thanks,
> Vladimir
> 
> On 10/28/16 12:07 AM, Lindenmaier, Goetz wrote:
> > 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.
> >>>>>>