Tamar Christina <tamar.christ...@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.
> 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
