Re: [PATCH v2] machine descriptor: New compact syntax for insn and insn_split in Machine Descriptions.

["Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>]

On 06/06/2023 13:49, Richard Sandiford via Gcc-patches wrote:
> Tamar Christina <Tamar.Christina@arm.com> writes:
>>>>     int operand_number;		/* Operand index in the big array.  */
>>>>     int output_format;		/* INSN_OUTPUT_FORMAT_*.  */
>>>> +  bool compact_syntax_p;
>>>>     struct operand_data operand[MAX_MAX_OPERANDS];  };
>>>>
>>>> @@ -700,12 +702,57 @@ process_template (class data *d, const char
>>> *template_code)
>>>>   	  if (sp != ep)
>>>>   	    message_at (d->loc, "trailing whitespace in output template");
>>>>
>>>> -	  while (cp < sp)
>>>> +	  /* Check for any unexpanded iterators.  */
>>>> +	  if (bp[0] != '*' && d->compact_syntax_p)
>>>
>>> I assume the bp[0] != '*' condition skips the check for C code blocks.
>>> Genuine question, but are you sure we want that?  C code often includes asm
>>> strings (in quotes), such as for the SVE CNT[BHWD] example.
>>>
>>> Extending the check would mean that any use of <...> for C++ templates will
>>> need to be quoted, but explicit instantiation is pretty rare in .md files.  It would
>>> also look weird for conditions.
>>>
>>> Either way is fine, just asking.
>>
>> I excluded it entirely to avoid also running afoul of the binary operators. So e.g.
>> * a < b && b > c ? foo : bar shouldn't trigger it.   It seemed more trouble than it's
>> worth to try to get correct.
> 
> Yeah.  I agree it's probably better to skip.
> 
>>>> +  }
>>>> +
>>>> +  /* Adds a character to the end of the string.  */  void add (char
>>>> + c)  {
>>>> +    con += c;
>>>> +  }
>>>> +
>>>> +  /* Output the string in the form of a brand-new char *, then effectively
>>>> +     clear the internal string by resetting len to 0.  */  char * out
>>>> + ()
>>>
>>> Formatting: no need for a space before "out".
>>>
>>>> +  {
>>>> +    /* Final character is always a trailing comma, so strip it out.
>>>> + */
>>>
>>> trailing ',', ';' or ']', rather than just a comma?
>>
>> Ah no, this is a bit of a lazy intercalate, when the alternatives are pushed in it's
>> not easy to tell how many there will be (because we don't keep track of it in this part),
>> so we just always add a trailing "," and ignore the last char on output.  Validation of the
>> alternative counts themselves is done later by the normal machinery.
> 
> Ah, I get it now, thanks.
> 
>>>> +    }
>>>> +
>>>> +  return index;
>>>> +}
>>>> +
>>>> +/* Modify the attributes list to make space for the implicitly declared
>>>> +   attributes in the attrs: list.  */
>>>> +
>>>> +static void
>>>> +create_missing_attributes (rtx x, file_location /* loc */,
>>>> +vec_conlist &attrs) {
>>>> +  if (attrs.empty ())
>>>> +    return;
>>>> +
>>>> +  unsigned int attr_index = GET_CODE (x) == DEFINE_INSN ? 4 : 3;
>>>> + vec_conlist missing;
>>>> +
>>>> +  /* This is an O(n*m) loop but it's fine, both n and m will always be very
>>>> +     small.  */
>>>
>>> Agreed that quadraticness isn't a problem.  But I wonder how many people
>>> would write an explicit placeholder set_attr.  Unlike match_operand and
>>> match_scratch, a placeholder set_attr doesn't carry any additional
>>> information.
>>>
>>> It might be simpler to drop add_attributes and add all attributes
>>> unconditionally in this function instead.  If the user tries to specify the same
>>> attribute using both syntaxes, the pattern would end up with two definitions
>>> of the same attribute, which ought to be flagged by existing code.
>>>
>>
>> This was done to support the (in arm backend) common thing of having attributes
>> which are either too complex to add inline in the new syntax or that just repeat a
>> value.
>>
>> i.e. it's to allow cases like this:
>>
>>    [(set_attr "length")
>>     (set_attr "predicable" "yes")
>>     (set_attr "predicable_short_it")
>>     (set_attr "arch")
>>     (set (attr "type") (if_then_else (match_operand 2 "const_int_operand" "")
>> 		      (const_string "alu_imm")
>> 		      (const_string "alu_sreg")))
>>
>> Where your attrs contains:
>>
>>    {@ [cons: =0, 1, 2; attrs: length, predicable_short_it, arch]
> 
> Yeah, agree it needs to be possible to define things like "type"
> in this way.

You also want it for the case where every alternative takes the same 
value, eg the "predicable - yes" attr.

R.

> 
>> However you're right, I could simply say that you must omit the set_attr in attrs and just
>> merge the two lists?  I think that's what you were alluding to?
> 
> Yeah, that's right.  Or just concatenate them and rely on later
> error checking (which should give reasonable diagnostics).
> 
> Thanks,
> Richard