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

[Vladimir Kozlov]

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.
>>>>>>