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

[Richard Sandiford <richard.sandiford@arm.com>]

In addition to Andreas's and Richard's comments:

Tamar Christina <Tamar.Christina@arm.com> writes:
> +@item
> +@samp{@{@@} is followed by a layout in parentheses which is @samp{cons:} followed by
> +a list of @code{match_operand}/@code{match_scratch} comma operand numbers, then a

How about:

  a comma-separated list of @code{match_operand}/@code{match_scratch} operand
  numbers, then a

Some lines are >80 chars.

> +semicolon, followed by the same for attributes (@samp{attrs:}).  Operand
> +modifiers can be placed in this section group as well.  Both sections
> +are optional (so you can use only @samp{cons}, or only @samp{attrs}, or both),
> +and @samp{cons} must come before @samp{attrs} if present.
> +
> +@item
> +Each alternative begins with any amount of whitespace.
> +
> +@item
> +Following the whitespace is a comma-separated list of "constraints" and/or
> +"attributes" within brackets @code{[]}, with sections separated by a semicolon.
> +
> +@item
> +Should you want to copy the previous asm line, the symbol @code{^} can be used.
> +This allows less copy pasting between alternative and reduces the number of
> +lines to update on changes.
> +
> +@item
> +When using C functions for output, the idiom @samp{* return <function>;} can be

@samp{* return @var{function};}

> +replaced with the shorthand @samp{<< @var{function};}.
> +
> +@item
> +Following the closing @samp{]} is any amount of whitespace, and then the actual
> +asm output.
> +
> +@item
> +Spaces are allowed in the list (they will simply be removed).
> +
> +@item
> +All constraint alternatives should be specified: a blank list should be
> +@samp{[,,]} or generally use @samp{*} for the alternatives. e.g. @samp{[*,*,*]}.

I think this is mixing two things.  How about:

@item
All constraint alternatives should be specified.  For example, a list of
of three blank alternatives should be written @samp{[,,]} rather than
@samp{[]}.

@item
All attribute alternatives should be non-empty, with @samp{*}
representing the default attribute value.  For example, a list of three
default attribute values should be written @samp{[*,*,*]} rather than
@samp{[]}.

> +
> +@item
> +Within an @samp{@{@@} block both multiline and singleline C comments are
> +allowed, but when used outside of a C block they must be the only non-whitespace
> +blocks on the line.
> +
> +@item
> +Within an @samp{@{@@} block, any iterators that do not get expanded will result in an
> +error.  If for some reason it is required to have @code{<>} in the output then

Maybe better as:

s/@code{<>}/@code{<} or @code{>}/

> +these must be escaped using @backslashchar{}.
> +
> +@item
> +The actual constraint string in the @code{match_operand} or
> +@code{match_scratch}, and the attribute string in the @code{set_attr}, must be
> +blank or an empty string (you can't combine the old and new syntaxes).

It looks like the new version drops support for the set_attr case though
(thanks).

> +
> +@item
> +Additional @code{set_attr} can be specified other than the ones in the
> +@samp{attrs} list.  These must use the normal syntax and must come last.  There
> +must not be any overlap between the two lists.

Similarly here: I don't think the “they must come last” bit applies
any more.  How about something like:

  It is possible to use the @samp{attrs} list to specify some attributes
  and to use the normal @code{set_attr} syntax to specify other attributes.
  There must not be any overlap between the two lists.

> +
> +In other words, the following is valid:
> +@smallexample
> +@group
> +(define_insn_and_split ""
> +  [(set (match_operand:SI 0 "nonimmediate_operand")
> +       (match_operand:SI 1 "aarch64_mov_operand"))]
> +  ""
> +  @{@@ [cons: 0, 1; attrs: type, arch, length]@}
> +  @dots{}
> +  [(set_attr "foo" "mov_imm")]
> +)
> +@end group
> +@end smallexample
> +
> +but these are not valid:
> +@smallexample
> +@group
> +(define_insn_and_split ""
> +  [(set (match_operand:SI 0 "nonimmediate_operand")
> +       (match_operand:SI 1 "aarch64_mov_operand"))]
> +  ""
> +  @{@@ [cons: 0, 1; attrs: type, arch, length]@}
> +  @dots{}
> +  [(set_attr "type")
> +   (set_attr "arch")
> +   (set_attr "foo" "mov_imm")]
> +)
> +@end group
> +@end smallexample
> +
> +and
> +
> +@smallexample
> +@group
> +(define_insn_and_split ""
> +  [(set (match_operand:SI 0 "nonimmediate_operand")
> +       (match_operand:SI 1 "aarch64_mov_operand"))]
> +  ""
> +  @{@@ [cons: 0, 1; attrs: type, arch, length]@}
> +  @dots{}
> +  [(set_attr "type")
> +   (set_attr "foo" "mov_imm")
> +   (set_attr "arch")
> +   (set_attr "length")]
> +)
> +@end group
> +@end smallexample
> +
> +because the order of the entries don't match and new entries must be last.
> +@end itemize

These examples probably need updating too.

> +
>  @node Predicates
>  @section Predicates
>  @cindex predicates
> diff --git a/gcc/genoutput.cc b/gcc/genoutput.cc
> index 163e8dfef4ca2c2c92ce1cf001ee6be40a54ca3e..7088f816cfa6e6ab2c1f51b8bbaa5eae990a0a4b 100644
> --- a/gcc/genoutput.cc
> +++ b/gcc/genoutput.cc
> @@ -157,6 +157,7 @@ public:
>    int n_alternatives;          /* Number of alternatives in each constraint */
>    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 +701,51 @@ 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)
>             {
> -             putchar (*cp);
> -             cp++;
> +             const char *p = cp;
> +             const char *last_bracket = nullptr;
> +             while (p < sp)
> +               {
> +                 if (*p == '\\' && p + 1 < sp)
> +                   {
> +                     putchar (*p);
> +                     putchar (*(p+1));
> +                     p += 2;
> +                     continue;
> +                   }
> +
> +                 if (*p == '>' && last_bracket && *last_bracket == '<')
> +                   {
> +                     int len = p - last_bracket;
> +                     fatal_at (d->loc, "unresolved iterator '%.*s' in '%s'",
> +                               len - 1, last_bracket + 1, cp);
> +                   }
> +                 else if (*p == '<' || *p == '>')
> +                   last_bracket = p;
> +
> +                 putchar (*p);
> +                 p += 1;
> +               }
> +
> +             if (last_bracket)
> +               {
> +                 char *nl = strchr (const_cast<char*> (cp), '\n');
> +                 if (nl)
> +                   *nl ='\0';

Nit: missing space after "=".

> +                 fatal_at (d->loc, "unmatched angle brackets, likely an "
> +                           "error in iterator syntax in %s", cp);
> +               }
> +           }
> +         else
> +           {
> +             while (cp < sp)
> +               putchar (*(cp++));
>             }
>
> +         cp = sp;
> +
>           if (!found_star)
>             puts ("\",");
>           else if (*bp != '*')
> @@ -881,6 +921,8 @@ gen_insn (md_rtx_info *info)
>    else
>      d->name = 0;
>
> +  d->compact_syntax_p = compact_syntax.contains (insn);
> +
>    /* Build up the list in the same order as the insns are seen
>       in the machine description.  */
>    d->next = 0;
> diff --git a/gcc/gensupport.h b/gcc/gensupport.h
> index a1edfbd71908b6244b40f801c6c01074de56777e..7925e22ed418767576567cad583bddf83c0846b1 100644
> --- a/gcc/gensupport.h
> +++ b/gcc/gensupport.h
> @@ -20,6 +20,7 @@ along with GCC; see the file COPYING3.  If not see
>  #ifndef GCC_GENSUPPORT_H
>  #define GCC_GENSUPPORT_H
>
> +#include "hash-set.h"
>  #include "read-md.h"
>
>  struct obstack;
> @@ -218,6 +219,8 @@ struct pattern_stats
>    int num_operand_vars;
>  };
>
> +extern hash_set<rtx> compact_syntax;
> +
>  extern void get_pattern_stats (struct pattern_stats *ranges, rtvec vec);
>  extern void compute_test_codes (rtx, file_location, char *);
>  extern file_location get_file_location (rtx);
> diff --git a/gcc/gensupport.cc b/gcc/gensupport.cc
> index f9efc6eb7572a44b8bb154b0b22be3815bd0d244..507285e8fef5443ec42501447e730022e1426ee1 100644
> --- a/gcc/gensupport.cc
> +++ b/gcc/gensupport.cc
> @@ -18,6 +18,8 @@
>     <http://www.gnu.org/licenses/>.  */
>
>  #include "bconfig.h"
> +#define INCLUDE_STRING
> +#define INCLUDE_VECTOR
>  #include "system.h"
>  #include "coretypes.h"
>  #include "tm.h"
> @@ -33,6 +35,8 @@
>  static rtx operand_data[MAX_OPERANDS];
>  static rtx match_operand_entries_in_pattern[MAX_OPERANDS];
>  static char used_operands_numbers[MAX_OPERANDS];
> +/* List of entries which are part of the new syntax.  */
> +hash_set<rtx> compact_syntax;
>
>
>  /* In case some macros used by files we include need it, define this here.  */
> @@ -545,6 +549,526 @@ gen_rewrite_sequence (rtvec vec)
>    return new_vec;
>  }
>
> +/* The following is for handling the compact syntax for constraints and
> +   attributes.
> +
> +   The normal syntax looks like this:
> +
> +       ...
> +       (match_operand: 0 "s_register_operand" "r,I,k")
> +       (match_operand: 2 "s_register_operand" "r,k,I")
> +       ...
> +       "@
> +       <asm>
> +       <asm>
> +       <asm>"
> +       ...
> +       (set_attr "length" "4,8,8")
> +
> +   The compact syntax looks like this:
> +
> +       ...
> +       (match_operand: 0 "s_register_operand")
> +       (match_operand: 2 "s_register_operand")
> +       ...
> +       {@ [cons: 0, 2; attrs: length]
> +       [r,r; 4] <asm>
> +       [I,k; 8] <asm>
> +       [k,I; 8] <asm>
> +       }
> +       ...
> +       (set_attr "length")

Similarly here, I think the final set_attr is no longer expected.

> +
> +   This is the only place where this syntax needs to be handled.  Relevant
> +   patterns are transformed from compact to the normal syntax before they are
> +   queued, so none of the gen* programs need to know about this syntax at all.
> +
> +   Conversion process (convert_syntax):
> +
> +   0) Check that pattern actually uses new syntax (check for {@ ... }).
> +
> +   1) Get the "layout", i.e. the "[cons: 0 2; attrs: length]" from the above
> +      example.  cons must come first; both are optional. Set up two vecs,
> +      convec and attrvec, for holding the results of the transformation.
> +
> +   2) For each alternative: parse the list of constraints and/or attributes,
> +      and enqueue them in the relevant lists in convec and attrvec.  By the end
> +      of this process, convec[N].con and attrvec[N].con should contain regular
> +      syntax constraint/attribute lists like "r,I,k".  Copy the asm to a string
> +      as we go.
> +
> +   3) Search the rtx and write the constraint and attribute lists into the
> +      correct places. Write the asm back into the template.  */
> +
> +/* Helper class for shuffling constraints/attributes in convert_syntax and
> +   add_constraints/add_attributes.  This includes commas but not whitespace.  */
> +
> +class conlist {
> +private:
> +  std::string con;
> +
> +public:
> +  std::string name;
> +  int idx = -1;
> +
> +  conlist () = default;
> +
> +  /* [ns..ns + len) should be a string with the id of the rtx to match
> +     i.e. if rtx is the relevant match_operand or match_scratch then
> +     [ns..ns + len) should equal itoa (XINT (rtx, 0)), and if set_attr then
> +     [ns..ns + len) should equal XSTR (rtx, 0).  */
> +  conlist (const char *ns, unsigned int len, bool numeric)
> +  {
> +    /* Trim leading whitespaces.  */
> +    while (ISSPACE (*ns))
> +      {
> +       ns++;
> +       len--;
> +      }
> +
> +    /* Trim trailing whitespace.  */
> +    for (int i = len - 1; i >= 0; i++, len--)
> +      if (!ISSPACE (*ns))
> +       break;

As you pointed out off-list, these should be ISBLANK rather than ISSPACE.
Sorry for missing the effect on '\n'.

> +    /* Parse off any modifiers.  */
> +    while (!ISALNUM (*ns))
> +      {
> +       con += *(ns++);
> +       len--;
> +      }
> +
> +    name.assign (ns, len);
> +    if (numeric)
> +      idx = std::stoi (name);
> +  }
> +
> +  /* 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 ()
> +  {
> +    /* Final character is always a trailing comma, so strip it out.  */
> +    char *q = xstrndup (con.c_str (), con.size () - 1);
> +    con.clear ();
> +    return q;
> +  }
> +};
> +
> +typedef std::vector<conlist> vec_conlist;
> +
> +/* Add constraints to an rtx.  This function is similar to remove_constraints.
> +   Errors if adding the constraints would overwrite existing constraints.  */
> +
> +static void
> +add_constraints (rtx part, file_location loc, vec_conlist &cons)
> +{
> +  const char *format_ptr;
> +
> +  if (part == NULL_RTX)
> +    return;
> +
> +  /* If match_op or match_scr, check if we have the right one, and if so, copy
> +     over the constraint list.  */
> +  if (GET_CODE (part) == MATCH_OPERAND || GET_CODE (part) == MATCH_SCRATCH)
> +    {
> +      int field = GET_CODE (part) == MATCH_OPERAND ? 2 : 1;
> +      unsigned id = XINT (part, 0);
> +
> +      if (id >= cons.size ())
> +       fatal_at (loc, "could not find match_operand/scratch with id %d", id);
> +
> +      if (cons[id].idx == -1)
> +       {
> +         error_at (loc, "constructor %d encountered more than once", id);
> +         return;
> +       }

-1 can also mean that the cons: didn't list "id" at all.  So if we want
to detect duplicates, we would need to assign something else (such as -2)
when consuming a constraint.  But I don't think it's necessary to check
for duplicates here, since later code should do that for both syntaxes.

So there are two options:

(1) Return early without error if "cons[id].idx == -1".
    Continue to use "cons[id].idx = -1" when consuming the constraint.
    
(2) Return early without error if "cons[id].idx == -1".
    Flag an error if "cons[id].idx == -2".
    Use "cons[id].idx = -2" when consuming the constraint.

I was thinking of (1), but (2) would also be OK if you prefer.

> +      if (XSTR (part, field)[0] != '\0')
> +       {
> +         error_at (loc, "can't mix normal and compact constraint syntax");
> +         return;
> +       }
> +      XSTR (part, field) = cons[id].out ();
> +      cons[id].idx = -1;
> +    }
> +
> +  format_ptr = GET_RTX_FORMAT (GET_CODE (part));
> +
> +  /* Recursively search the rtx.  */
> +  for (int i = 0; i < GET_RTX_LENGTH (GET_CODE (part)); i++)
> +    switch (*format_ptr++)
> +      {
> +      case 'e':
> +      case 'u':
> +       add_constraints (XEXP (part, i), loc, cons);
> +       break;
> +      case 'E':
> +       if (XVEC (part, i) != NULL)
> +         for (int j = 0; j < XVECLEN (part, i); j++)
> +           add_constraints (XVECEXP (part, i, j), loc, cons);
> +       break;
> +      default:
> +       continue;
> +      }
> +}
> +
> +/* Add attributes to an rtx. The attributes that are matched must be in order
> +   i.e. read from top to bottom in the pattern.
> +   Errors if adding the attributes would overwrite existing attributes.
> +   Returns 1 + index of last conlist to be matched.  */
> +
> +static unsigned int
> +add_attributes (rtx x, file_location loc, vec_conlist &attrs)
> +{
> +  unsigned int attr_index = GET_CODE (x) == DEFINE_INSN ? 4 : 3;
> +  unsigned int index = 0;
> +
> +  if (XVEC (x, attr_index) == NULL)
> +    return index;
> +
> +  for (int i = 0; i < XVECLEN (x, attr_index); ++i)
> +    {
> +      rtx part = XVECEXP (x, attr_index, i);
> +
> +      if (GET_CODE (part) != SET_ATTR)
> +       continue;
> +
> +      if (attrs[index].name == XSTR (part, 0))
> +       {
> +         if (XSTR (part, 1) && XSTR (part, 1)[0] != '\0')
> +           fatal_at (loc, "can't mix normal and compact attribute syntax");
> +         XSTR (part, 1) = attrs[index].out ();
> +
> +         if (++index == attrs.size ())
> +           break;
> +       }
> +      else
> +       fatal_at (loc, "can't mix normal and compact attribute syntax");
> +    }
> +
> +  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;
> +  rtvec orig = XVEC (x, attr_index);
> +  size_t n_curr = orig ? XVECLEN (x, attr_index) : 0;
> +  rtvec copy = rtvec_alloc (n_curr + attrs.size ());
> +
> +  /* Create a shallow copy of existing entries.  */
> +  memcpy (&copy->elem[attrs.size ()], &orig->elem[0], sizeof (rtx) * n_curr);
> +  XVEC (x, attr_index) = copy;
> +
> +  /* Create the new elements.  */
> +  for (unsigned i = 0; i < attrs.size (); i++)
> +    {
> +      rtx attr = rtx_alloc (SET_ATTR);
> +      XSTR (attr, 0) = xstrdup (attrs[i].name.c_str ());
> +      XSTR (attr, 1) = NULL;
> +      XVECEXP (x, attr_index, i) = attr;
> +    }
> +
> +  return;
> +}

I think we should be able to combine the last two functions into
something like:

/* Add ATTRS to definition X's attribute list.  */

static void
add_attributes (rtx x, vec_conlist &attrs)
{
  unsigned int attr_index = GET_CODE (x) == DEFINE_INSN ? 4 : 3;
  rtvec orig = XVEC (x, attr_index);
  size_t n_curr = orig ? XVECLEN (x, attr_index) : 0;
  rtvec copy = rtvec_alloc (n_curr + attrs.size ());

  /* Create a shallow copy of existing entries.  */
  memcpy (&copy->elem[attrs.size ()], &orig->elem[0], sizeof (rtx) * n_curr);
  XVEC (x, attr_index) = copy;

  /* Create the new elements.  */
  for (unsigned i = 0; i < attrs.size (); i++)
    {
      rtx attr = rtx_alloc (SET_ATTR);
      XSTR (attr, 0) = xstrdup (attrs[i].name.c_str ());
      XSTR (attr, 1) = attrs[i].out ();
      XVECEXP (x, attr_index, i) = attr;
    }
}

Then drop the call to create_missing_attributes and replace:

  if (attrvec.size () > 0)
    {
      index = add_attributes (x, loc, attrvec);
      if (index < attrvec.size ())
       fatal_at (loc, "could not find set_attr for attribute %s",
                 attrvec[index].name.c_str ());
    }

with:

  if (attrvec.size () > 0)
    add_attributes (x, attrs);

> +
> +/* Consumes spaces and tabs.  */
> +
> +static inline void
> +skip_spaces (const char **str)
> +{
> +  while (ISSPACE (**str))

ISBLANK here too (sorry).

> +    (*str)++;
> +}
> +
> +/* Consumes the given character, if it's there.  */
> +
> +static inline bool
> +expect_char (const char **str, char c)
> +{
> +  if (**str != c)
> +    return false;
> +  (*str)++;
> +  return true;
> +}
> +
> +/* Parses the section layout that follows a "{@}" if using new syntax. Builds

"{@"

> +   a vector for a single section. E.g. if we have "attrs: length arch]..."

"attrs: length, arch]..."

> +   then list will have two elements, the first for "length" and the second
> +   for "arch".  */
> +
> +static void
> +parse_section_layout (const char **templ, const char *label,
> +                     vec_conlist &list, bool numeric)
> +{
> +  const char *name_start;
> +  size_t label_len = strlen (label);
> +  if (strncmp (label, *templ, label_len) == 0)
> +    {
> +      *templ += label_len;
> +
> +      /* Gather the names.  */
> +      while (**templ != ';' && **templ != ']')
> +       {
> +         skip_spaces (templ);
> +         name_start = *templ;
> +         int len = 0;
> +         char val = (*templ)[len];
> +         while (val != ',' && val != ';' && val != ']')
> +            val = (*templ)[++len];

Sorry for not noticing last time, but this loop should raise an error
if val is 0 or '\n', before the assignment of a new val.  Otherwise a
malformed string would lead to a segfault.  E.g. something like:

         while (val != ',' && val != ';' && val != ']')
           {
	     if (val == 0 || val == '\n')
               fatal_at (loc, "missing ']'");
	     val = (*templ)[++len];
	   }

> +         *templ += len;
> +         if (val == ',')
> +           (*templ)++;
> +         list.push_back (conlist (name_start, len, numeric));
> +       }
> +    }
> +}
> +
> +/* Parse a section, a section is defined as a named space separated list, e.g.
> +
> +   foo: a, b, c
> +
> +   is a section named "foo" with entries a, b and c.  */
> +
> +static void
> +parse_section (const char **templ, unsigned int n_elems, unsigned int alt_no,
> +              vec_conlist &list, file_location loc, const char *name)
> +{
> +  unsigned int i;
> +
> +  /* Go through the list, one character at a time, adding said character
> +     to the correct string.  */
> +  for (i = 0; **templ != ']' && **templ != ';'; (*templ)++)
> +    {
> +      if (!ISSPACE (**templ))

ISBLANK here too.  No need for the "for" loop to have braces.

> +       {

Here too we should check **templ for 0 or '\n', and raise an error if found.

> +         list[i].add(**templ);

Nit: should be a space after "add".

> +         if (**templ == ',')
> +           {
> +             ++i;
> +             if (i == n_elems)
> +               fatal_at (loc, "too many %ss in alternative %d: expected %d",
> +                         name, alt_no, n_elems);
> +           }
> +       }
> +    }
> +
> +  if (i + 1 < n_elems)
> +    fatal_at (loc, "too few %ss in alternative %d: expected %d, got %d",
> +             name, alt_no, n_elems, i);
> +
> +  list[i].add(',');

Nit: should be a space after "add".

> +}
> +
> +/* The compact syntax has more convience syntaxes.  As such we post process
> +   the lines to get them back to something the normal syntax understands.  */
> +
> +static void
> +preprocess_compact_syntax (file_location loc, int alt_no, std::string &line,
> +                          std::string &last_line)
> +{
> +  /* Check if we're copying the last statement.  */
> +  if (line.find ("^") == 0 && line.size () == 1)
> +    {
> +      if (last_line.empty ())
> +       fatal_at (loc, "found instruction to copy previous line (^) in"
> +                      "alternative %d but no previous line to copy", alt_no);
> +      line = last_line;
> +      return;
> +    }
> +
> +  std::string result;
> +  std::string buffer;
> +  /* Check if we have << which means return c statement.  */
> +  if (line.find ("<<") == 0)
> +    {
> +      result.append ("* return ");
> +      const char *chunk = line.c_str () + 2;
> +      skip_spaces (&chunk);
> +      result.append (chunk);
> +    }
> +  else
> +    result.append (line);
> +
> +  line = result;
> +  return;
> +}
> +
> +/* Converts an rtx from compact syntax to normal syntax if possible.  */
> +
> +static void
> +convert_syntax (rtx x, file_location loc)
> +{
> +  int alt_no;
> +  unsigned int index, templ_index;
> +  const char *templ;
> +  vec_conlist tconvec, convec, attrvec;
> +
> +  templ_index = GET_CODE (x) == DEFINE_INSN ? 3 : 2;
> +
> +  templ = XTMPL (x, templ_index);
> +
> +  /* Templates with constraints start with "{@".  */
> +  if (strncmp ("*{@", templ, 3))
> +    return;
> +
> +  /* Get the layout for the template.  */
> +  templ += 3;
> +  skip_spaces (&templ);
> +
> +  if (!expect_char (&templ, '['))
> +    fatal_at (loc, "expecing `[' to begin section list");
> +
> +  parse_section_layout (&templ, "cons:", tconvec, true);
> +  convec.resize (tconvec.size ());
> +
> +  /* Check for any duplicate cons entries and sort based on i.  */
> +  for (auto e : tconvec)
> +    {
> +      unsigned idx = e.idx;
> +      if (idx >= convec.size ())
> +       convec.resize (idx + 1);
> +
> +      if (convec[idx].idx >= 0)
> +       fatal_at (loc, "duplicate cons number found: %d", idx);
> +      convec[idx] = e;
> +    }
> +  tconvec.clear ();
> +
> +  if (*templ != ']')
> +    {
> +      if (*templ == ';')
> +       skip_spaces (&(++templ));
> +      parse_section_layout (&templ, "attrs:", attrvec, false);
> +      create_missing_attributes (x, loc, attrvec);
> +    }
> +
> +  if (!expect_char (&templ, ']'))
> +    fatal_at (loc, "expecting `]` to end section list - section list must have "
> +                  "cons first, attrs second");
> +
> +  /* We will write the un-constrainified template into new_templ.  */
> +  std::string new_templ;
> +  new_templ.append ("@");
> +
> +  /* Skip to the first proper line.  */
> +  skip_spaces (&templ);
> +
> +  alt_no = 0;
> +  std::string last_line;
> +
> +  /* Process the alternatives.  */
> +  while (*(templ - 1) != '\0')
> +    {
> +      /* Skip leading whitespace.  */
> +      std::string buffer;
> +      skip_spaces (&templ);
> +
> +      /* Check if we're at the end.  */
> +      if (templ[0] == '}' && templ[1] == '\0')
> +       break;
> +
> +      new_templ += '\n';
> +      new_templ.append (buffer);

Shouldn't this be...

> +
> +      if (expect_char (&templ, '['))
> +       {

...here, to prevent an extra '\n' from being added if the template
ends with a comment?

> +         /* Parse the constraint list, then the attribute list.  */
> +         if (convec.size () > 0)
> +           parse_section (&templ, convec.size (), alt_no, convec, loc,
> +                          "constraint");
> +
> +         if (attrvec.size () > 0)
> +           {
> +             if (convec.size () > 0 && !expect_char (&templ, ';'))
> +               fatal_at (loc, "expected `;' to separate constraints "
> +                              "and attributes in alternative %d", alt_no);
> +
> +             parse_section (&templ, attrvec.size (), alt_no,
> +                            attrvec, loc, "attribute");
> +           }
> +
> +         if (!expect_char (&templ, ']'))
> +           fatal_at (loc, "expected end of constraint/attribute list but "
> +                          "missing an ending `]' in alternative %d", alt_no);
> +       }
> +      else if (templ[0] == '/' && templ[1] == '/')
> +       {
> +         templ += 2;
> +         /* Glob till newline or end of string.  */
> +         while (*templ != '\n' || *templ != '\0')
> +           templ++;
> +         templ++;
> +         continue;
> +       }
> +      else if (templ[0] == '/' && templ[1] == '*')
> +       {
> +         templ += 2;
> +         /* Glob till newline or end of multiline comment.  */
> +         while (templ[0] != '*' && templ[1] != '/')
> +           templ++;

This loop should check for templ[0] == 0 too.

> +         templ += 2;
> +         continue;
> +       }
> +      else
> +       fatal_at (loc, "expected constraint/attribute list at beginning of "
> +                      "alternative %d but missing a starting `['", alt_no);
> +
> +      /* Skip whitespace between list and asm.  */
> +      ++templ;
> +      skip_spaces (&templ);
> +
> +      /* Copy asm to new template.  */
> +      std::string line;
> +      while (*templ != '\n' && *templ != '\0')
> +       line += *templ++;
> +
> +      /* Apply any pre-processing needed to the line.  */
> +      preprocess_compact_syntax (loc, alt_no, line, last_line);
> +      new_templ.append (line);
> +      last_line = line;
> +
> +      /* Normal "*..." syntax expects the closing quote to be on the final
> +        line of asm, whereas we allow the closing "}" to be on its own line.
> +        Postpone copying the '\n' until we know that there is another
> +        alternative in the list.  */
> +      while (templ[0] == '\n' || ISSPACE (*templ))
> +       templ++;

The separate check for '\n' isn't necessary.

Sorry for all the micro-comments.

Thanks,
Richard

> +      ++alt_no;
> +    }
> +
> +  /* Write the constraints and attributes into their proper places.  */
> +  if (convec.size () > 0)
> +    add_constraints (x, loc, convec);
> +
> +  if (attrvec.size () > 0)
> +    {
> +      index = add_attributes (x, loc, attrvec);
> +      if (index < attrvec.size ())
> +       fatal_at (loc, "could not find set_attr for attribute %s",
> +                 attrvec[index].name.c_str ());
> +    }
> +
> +  /* Copy over the new un-constrainified template.  */
> +  XTMPL (x, templ_index) = xstrdup (new_templ.c_str ());
> +
> +  /* Register for later checks during iterator expansions.  */
> +  compact_syntax.add (x);
> +}
> +
>  /* Process a top level rtx in some way, queuing as appropriate.  */
>
>  static void
> @@ -553,10 +1077,12 @@ process_rtx (rtx desc, file_location loc)
>    switch (GET_CODE (desc))
>      {
>      case DEFINE_INSN:
> +      convert_syntax (desc, loc);
>        queue_pattern (desc, &define_insn_tail, loc);
>        break;
>
>      case DEFINE_COND_EXEC:
> +      convert_syntax (desc, loc);
>        queue_pattern (desc, &define_cond_exec_tail, loc);
>        break;
>
> @@ -631,6 +1157,7 @@ process_rtx (rtx desc, file_location loc)
>         attr = XVEC (desc, split_code + 1);
>         PUT_CODE (desc, DEFINE_INSN);
>         XVEC (desc, 4) = attr;
> +       convert_syntax (desc, loc);
>
>         /* Queue them.  */
>         insn_elem = queue_pattern (desc, &define_insn_tail, loc);