Tamar Christina <[email protected]> writes:
> Today output operand modifiers are limited to a single character, as in
> %w0 or %c1. That works, but it makes the modifier namespace very small
> and encourages target ports either to use cryptic letters or to fall back
> to C output blocks for cases that are really just target-specific
> printing.
>
> This patch adds a longer spelling for the same mechanism:
>
> %[name]0
>
> The bracketed spelling is intentionally familiar rather than completely new:
> extended asm already uses %[Value] to refer to an operand with a symbolic
> name,
> as in:
>
> asm ("add %[out], %[in], %[imm]"
> : [out] "=r" (out)
> : [in] "r" (in), [imm] "I" (1));
>
> That existing form keeps its meaning. A bare %[Value] is still resolved by
> the front end as a symbolic operand reference. The new print-operand form is
> recognized when the closing bracket is followed by an operand number, as in
> %[name]0.
>
> The modifier syntax can also be combined with symbolic operand names. In user
> asm, %[modifier:Value] means "print operand Value using the multi-character
> modifier named modifier". The front end resolves Value to an operand number,
> so final output sees the same thing it would have seen for %[modifier:N]. For
> example:
>
> asm ("mov %[w:out], %[w:in]"
> : [out] "=r" (out)
> : [in] "r" (in));
>
> is resolved as if the template had used %[w]0 and %[w]1.
>
> GCC parses the bracketed modifier name, interns it, and passes PRINT_OPERAND
> an
> opaque integer code for it. Old-style modifiers keep their existing values,
> so
> existing target hooks continue to see 'w', 'c', etc. for the single-character
> forms. Backends that want to handle the new form can call
> output_operand_modifier_name (code) to recover "name".
>
> For example, a machine description can now write something like:
>
> "extract\t%[lowpart]0, %1"
>
> or if preferred
>
> "extract\t%[lowpart:0], %1"
>
> and the target's PRINT_OPERAND hook can handle "lowpart" as a named modifier.
> The parser also allows comma-separated nonempty components in the brackets,
> but
> deliberately leaves the interpretation of those components to the target.
>
> The patch also records whether an extended modifier came from user asm or from
> an internal machine-description output template. Targets can use
> output_operand_modifier_user_p (code) to reject internal-only names in
> user-facing asm while still using the clearer names internally.
>
> This addresses Richard Earnshaw's suggestion by keeping the target hook
> interface as an int code and mapping the longer spelling onto values outside
> the single-character range. Targets that do not opt in do not need a new hook
> or a new parser path, and existing hard-coded character uses continue to work.
>
> It also addresses Richard Sandiford's feedback by giving the new %[...] syntax
> its own namespace rather than automatically exposing every single-letter
> modifier through the new spelling. The user-asm bit lets a target decide
> which
> named modifiers are public and which are only for compiler-generated
> templates.
> The comma-separated spelling is accepted as a target-defined modifier string,
> so a port can later treat, say, %[gpr,zero]0 as a composition of
> target-specific printing choices without requiring more middle-end syntax.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu,
> arm-none-linux-gnueabihf, x86_64-pc-linux-gnu
> -m32, -m64 and no issues.
>
> OK for master?
>
> Thanks,
> Tamar
Thanks for doing this. It mostly looks good to me FWIW, but some
comments below:
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index
> 88213d25a899ed5bff25af732220c54d2359b492..1e6f5ddb0a815101f6b6c57bc8f610766e035895
> 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -12057,7 +12057,14 @@ instead of simply @samp{%2}).
> @ref{GenericOperandmodifiers,
> Generic Operand modifiers} lists the modifiers that are available
> on all targets. Other modifiers are hardware dependent.
> For example, the list of supported modifiers for x86 is found at
> -@ref{x86Operandmodifiers,x86 Operand modifiers}.
> +@ref{x86Operandmodifiers,x86 Operand modifiers}. Targets can also provide
> +multi-character modifiers, written as @samp{%[@var{modifier}:2]}.
> +
Very minor, but: I think it'd read better if this was part of the following
paragraph instead, so that it's more obvious that new paragraph is referring
specifically to the new syntax.
> +If the operand has a symbolic name, write
> +@samp{%[@var{modifier}:@var{asmSymbolicName}]}. The older spelling
> +@samp{%[@var{modifier}]2} is also accepted for numbered operands.
> +@samp{%[@var{name}]} without a modifier separator continues to mean the
> +operand named @var{name}.
>
> If the C code that follows the @code{asm} makes no use of any of the output
> operands, use @code{volatile} for the @code{asm} statement to prevent the
> [...]
> diff --git a/gcc/final.cc b/gcc/final.cc
> index
> b6be9be99b7f12d69345de3c4edf99d33dfa6087..9953e1d44d9d66672b5daacc5a6200de3ba1df8c
> 100644
> --- a/gcc/final.cc
> +++ b/gcc/final.cc
> @@ -84,6 +84,7 @@ along with GCC; see the file COPYING3. If not see
> #include "common/common-target.h"
> #include "diagnostic.h"
> #include "diagnostics/file-cache.h"
> +#include "selftest.h"
>
> #include "dwarf2out.h"
>
> @@ -3395,6 +3396,216 @@ do_assembler_dialects (const char *p, int *dialect)
> }
> #endif
>
> +/* Return true if the half-open range [START, END) contains only decimal
> + digits, parsing it as an operand number into OPNUM. */
> +
> +static bool
> +parse_operand_number (const char *start, const char *end,
> + unsigned long &opnum)
It'd be good to export this and reuse it instead of asm_operand_number_p.
Maybe that would mean moving it too, although I'm not sure where.
> +{
> + if (start == end)
> + return false;
> +
> + unsigned long result = 0;
> + for (const char *q = start; q != end; ++q)
> + {
> + if (!ISDIGIT (*q))
> + return false;
> + result = result * 10 + (*q - '0');
> + }
> + opnum = result;
> + return true;
> +}
> [...]
> +#if CHECKING_P
> +
> +namespace selftest {
> +
> +static void
> +test_parse_bracketed_operand ()
> +{
> + bracketed_operand operand;
> + const char *p = "[10]";
> + ASSERT_TRUE (parse_bracketed_operand (&p, operand));
> + ASSERT_FALSE (operand.has_modifier);
> + ASSERT_EQ (10UL, operand.opnum);
> + ASSERT_EQ ('\0', *p);
> +
> + p = "[fmt:11]";
> + ASSERT_TRUE (parse_bracketed_operand (&p, operand));
> + ASSERT_TRUE (operand.has_modifier);
> + ASSERT_TRUE (operand.modifier == "fmt");
> + ASSERT_EQ (11UL, operand.opnum);
> + ASSERT_EQ ('\0', *p);
> +
> + p = "[fmt]12";
> + ASSERT_TRUE (parse_bracketed_operand (&p, operand));
> + ASSERT_TRUE (operand.has_modifier);
> + ASSERT_TRUE (operand.modifier == "fmt");
> + ASSERT_EQ (12UL, operand.opnum);
> + ASSERT_EQ ('\0', *p);
> +
> + p = "[gpr,zero:13]";
> + ASSERT_TRUE (parse_bracketed_operand (&p, operand));
> + ASSERT_TRUE (operand.has_modifier);
> + ASSERT_TRUE (operand.modifier == "gpr,zero");
> + ASSERT_EQ (13UL, operand.opnum);
> + ASSERT_EQ ('\0', *p);
> +
> + p = "[fmt]";
> + ASSERT_FALSE (parse_bracketed_operand (&p, operand));
> + p = "[fmt:]";
> + ASSERT_FALSE (parse_bracketed_operand (&p, operand));
> + p = "[]0";
> + ASSERT_FALSE (parse_bracketed_operand (&p, operand));
> +}
> +
> +void
> +final_cc_tests ()
> +{
> + test_parse_bracketed_operand ();
> +}
> +
> +} // namespace selftest
> +
> +#endif /* CHECKING_P */
I think self-tests are conventially at the end of the file. Putting it
in the middle would artificially restrict where the code under test can go.
> @@ -3488,11 +3699,37 @@ output_asm_insn (const char *templ, rtx *operands)
> p++;
> fprintf (asm_out_file, "%d", insn_counter);
> }
> + else if (*p == '[')
> + {
> + bracketed_operand operand;
> + if (!parse_bracketed_operand (&p, operand))
> + output_operand_lossage ("invalid %%[...] operand modifier");
> +
> + unsigned long opnum = operand.opnum;
> + if (this_is_asm_operands && opnum >= insn_noperands)
> + {
> + /* Force the opnum in bounds to a bogus location. */
> + opnum = MAX_RECOG_OPERANDS;
> + output_operand_lossage ("operand number out of range");
> + }
> + else if (operand.has_modifier)
> + output_operand (operands[opnum],
> + output_operand_modifier_code
> + (operand.modifier, this_is_asm_operands));
> + else
> + output_operand (operands[opnum], 0);
> +
> + if (!opoutput[opnum])
> + oporder[ops++] = opnum;
> + opoutput[opnum] = 1;
> +
> + c = *p;
> + }
> /* % followed by a letter and some digits
> outputs an operand in a special way depending on the letter.
> Letters `acln' are implemented directly.
> Other letters are passed to `output_operand' so that
> - the TARGET_PRINT_OPERAND hook can define them. */
> + the selected target operand hook can define them. */
Is this comment change left over from an earlier version? I wasn't sure
what the "selected target operand hook" was referring to.
> else if (ISALPHA (*p))
> {
> int letter = *p++;
> @@ -3522,7 +3759,7 @@ output_asm_insn (const char *templ, rtx *operands)
> if (letter2 == 'c' || CONSTANT_ADDRESS_P (operands[opnum]))
> output_addr_const (asm_out_file, operands[opnum]);
> else
> - output_operand (operands[opnum], 'c');
> + output_operand (operands[opnum], letter);
> }
> else if (letter == 'n')
> {
> @@ -3570,9 +3807,12 @@ output_asm_insn (const char *templ, rtx *operands)
> }
> /* % followed by punctuation: output something for that
> punctuation character alone, with no operand. The
> - TARGET_PRINT_OPERAND hook decides what is actually done. */
> + selected target operand hook decides what is actually done. */
> else if (targetm.asm_out.print_operand_punct_valid_p ((unsigned char)
> *p))
> - output_operand (NULL_RTX, *p++);
> + {
> + const char *ptr = p++;
> + output_operand (NULL_RTX, *ptr);
> + }
> else
> output_operand_lossage ("invalid %%-code");
> break;
Same question for the changes in the two hunks above. I wasn't sure why
they were necessary.
> [...]
> @@ -870,6 +875,67 @@ check_unique_operand_names (tree outputs, tree inputs,
> tree labels)
> return false;
> }
>
> +/* P points to a '%' in an asm template. Return the '[' for the symbolic
> + operand name that should be resolved, or null if this % code does not
> + contain a symbolic operand name.
> +
> + The %[...] syntax has two uses in user asm: %[name] refers to a named
> + operand, whereas %[modifier:N] and %[modifier]N are multi-character output
> + modifiers. When both a modifier and a symbolic operand name are needed,
> + use %[modifier:name]. */
> +
> +static bool
> +asm_operand_number_p (const char *start, const char *end)
> +{
> + if (start == end)
> + return false;
> +
> + for (const char *q = start; q != end; ++q)
> + if (!ISDIGIT (*q))
> + return false;
> + return true;
> +}
> +
Missing function comment.
> +static const char *
> +asm_operand_name_start (const char *p)
> +{
> + gcc_checking_assert (p[0] == '%');
> +
> + if (p[1] == '[')
> + {
> + const char *q = strchr (p + 2, ']');
> + if (q)
> + {
> + const char *colon = nullptr;
> + for (const char *r = p + 2; r != q; ++r)
> + if (*r == ':')
> + {
> + colon = r;
> + break;
> + }
> +
> + if (colon)
> + {
> + if (asm_operand_number_p (colon + 1, q))
> + return NULL;
> + return colon + 1;
> + }
> +
> + if (asm_operand_number_p (p + 2, q))
> + return NULL;
> + if (ISDIGIT (q[1]))
> + return NULL;
I was going to suggest that we disallow the trailing digit syntax in
asms (and perhaps then in general), since it would reinterpret what
"%[name]0" means. However, it seems that what "%[name]0" currently
means isn't useful: "%[name]" is replaced with "%<opnum>" and so we get
a double-digit operand number. For example:
void f() {
asm volatile ("foo %[number]0" :: [x] "i" (1), [number] "r" (100));
}
is treated as equivalent to "foo %10" and so gives an out-of-range error.
Still, I'm somewhat sceptical of the "%[modifier]opno" syntax in general.
It's a new syntax with no existing users and I don't see why it would
ever be preferred over the more explicit %[modifier:opno]".
> + }
> + return p + 1;
> + }
> +
> + if (ISALPHA (p[1])
> + && (p[2] == '[' || (ISALPHA (p[2]) && p[3] == '[')))
> + return p + 2 + (p[2] != '[');
Heh. I hadn't realised that we were so loose about where the modifier ended.
But clearly that's pre-existing and the patch is doing the right thing by
preserving it.
> +
> + return NULL;
> +}
> +
> /* Resolve the names of the operands in *POUTPUTS and *PINPUTS to numbers,
> and replace the name expansions in STRING and in the constraints to
> those numbers. This is generally done in the front end while creating
> @@ -905,10 +971,7 @@ resolve_asm_operand_names (tree string, tree outputs,
> tree inputs, tree labels)
> c = TREE_STRING_POINTER (string);
> while ((c = strchr (c, '%')) != NULL)
> {
> - if (c[1] == '[')
> - break;
> - else if (ISALPHA (c[1])
> - && (c[2] == '[' || (ISALPHA (c[2]) && c[3] == '[')))
> + if (asm_operand_name_start (c))
> break;
> else
> {
> @@ -920,26 +983,23 @@ resolve_asm_operand_names (tree string, tree outputs,
> tree inputs, tree labels)
> if (c)
> {
> /* OK, we need to make a copy so we can perform the substitutions.
> - Assume that we will not need extra space--we get to remove '['
> - and ']', which means we cannot have a problem until we have more
> - than 999 operands. */
> - buffer = xstrdup (TREE_STRING_POINTER (string));
> + Allocate some spare room for %[modifier:name], where the replacement
> + number can be longer than NAME. */
> + size_t length = strlen (TREE_STRING_POINTER (string));
> + buffer = XNEWVEC (char, length * 2 + 1);
Not sure about the * 2. More below.
> + memcpy (buffer, TREE_STRING_POINTER (string), length + 1);
> p = buffer + (c - TREE_STRING_POINTER (string));
>
> while ((p = strchr (p, '%')) != NULL)
> {
> - if (p[1] == '[')
> - p += 1;
> - else if (ISALPHA (p[1]) && p[2] == '[')
> - p += 2;
> - else if (ISALPHA (p[1]) && ISALPHA (p[2]) && p[3] == '[')
> - p += 3;
> - else
> + const char *name_start = asm_operand_name_start (p);
> + if (!name_start)
> {
> p += 1 + (p[1] == '%');
> continue;
> }
>
> + p += name_start - p;
Isn't that just "p = name_start;"? If so, we could use name_start directly
in the statement below instead.
> p = resolve_operand_name_1 (p, outputs, inputs, labels);
Since this patch is rewriting this code anyway, it would be good to make
it work regardless of MAX_RECOG_OPERANDS (which won't exist once the
wasm backend goes in).
So I think at this point we should just abandon the preallocation approach
and use an obstack instead. That is: iterate over the original input
string, growing the obstack with output characters as we go. That
would avoid the need to calculate a length in advance, would cope
with any number of digits, and would remove the complex shuffle in
resolve_operand_name_1.
Thanks,
Richard
> }
>
> @@ -958,24 +1018,40 @@ resolve_asm_operand_names (tree string, tree outputs,
> tree inputs, tree labels)
> static char *
> resolve_operand_name_1 (char *p, tree outputs, tree inputs, tree labels)
> {
> - char *q;
> + char *q, *name_start, *replace_start, *replace_end;
> int op, op_inout;
> tree t;
>
> - /* Collect the operand name. */
> - q = strchr (++p, ']');
> + /* Collect the operand name. P either points to the '[' in %[name],
> + or to the first character of NAME in %[modifier:name]. */
> + if (*p == '[')
> + {
> + replace_start = p;
> + name_start = p + 1;
> + q = strchr (name_start, ']');
> + replace_end = q ? q + 1 : nullptr;
> + }
> + else
> + {
> + replace_start = p;
> + name_start = p;
> + q = strchr (name_start, ']');
> + replace_end = q;
> + }
> +
> if (!q)
> {
> error ("missing close brace for named operand");
> - return strchr (p, '\0');
> + return strchr (name_start, '\0');
> }
> + char end_marker = *q;
> *q = '\0';
>
> /* Resolve the name to a number. */
> for (op_inout = op = 0, t = outputs; t ; t = TREE_CHAIN (t), op++)
> {
> tree name = TREE_PURPOSE (TREE_PURPOSE (t));
> - if (name && strcmp (TREE_STRING_POINTER (name), p) == 0)
> + if (name && strcmp (TREE_STRING_POINTER (name), name_start) == 0)
> goto found;
> tree constraint = TREE_VALUE (TREE_PURPOSE (t));
> if (constraint && strchr (TREE_STRING_POINTER (constraint), '+') !=
> NULL)
> @@ -984,35 +1060,89 @@ resolve_operand_name_1 (char *p, tree outputs, tree
> inputs, tree labels)
> for (t = inputs; t ; t = TREE_CHAIN (t), op++)
> {
> tree name = TREE_PURPOSE (TREE_PURPOSE (t));
> - if (name && strcmp (TREE_STRING_POINTER (name), p) == 0)
> + if (name && strcmp (TREE_STRING_POINTER (name), name_start) == 0)
> goto found;
> }
> op += op_inout;
> for (t = labels; t ; t = TREE_CHAIN (t), op++)
> {
> tree name = TREE_PURPOSE (t);
> - if (name && strcmp (TREE_STRING_POINTER (name), p) == 0)
> + if (name && strcmp (TREE_STRING_POINTER (name), name_start) == 0)
> goto found;
> }
>
> - error ("undefined named operand %qs", identifier_to_locale (p));
> + error ("undefined named operand %qs", identifier_to_locale (name_start));
> op = 0;
>
> found:
> - /* Replace the name with the number. Unfortunately, not all libraries
> - get the return value of sprintf correct, so search for the end of the
> - generated string by hand. */
> - sprintf (--p, "%d", op);
> - p = strchr (p, '\0');
> + *q = end_marker;
> + char number[HOST_BITS_PER_INT * 3 / 10 + 3];
> + sprintf (number, "%d", op);
> + size_t number_len = strlen (number);
> + memmove (replace_start + number_len, replace_end,
> + strlen (replace_end) + 1);
> + memcpy (replace_start, number, number_len);
> +
> + return replace_start + number_len;
> +}
> +
> +#if CHECKING_P
> +
> +namespace selftest {
> +
> +static tree
> +build_asm_operand (const char *name, const char *constraint)
> +{
> + tree name_tree = name ? build_string (strlen (name), name) : NULL_TREE;
> + tree constraint_tree = build_string (strlen (constraint), constraint);
> + return tree_cons (tree_cons (name_tree, constraint_tree, NULL_TREE),
> + integer_zero_node, NULL_TREE);
> +}
>
> - /* Verify the no extra buffer space assumption. */
> - gcc_assert (p <= q);
> +static tree
> +build_asm_label (const char *name)
> +{
> + return tree_cons (build_string (strlen (name), name), integer_zero_node,
> + NULL_TREE);
> +}
>
> - /* Shift the rest of the buffer down to fill the gap. */
> - memmove (p, q + 1, strlen (q + 1) + 1);
> +static void
> +test_resolve_asm_operand_names ()
> +{
> + tree outputs = build_asm_operand ("out", "=r");
> + tree inputs = build_asm_operand ("in", "r");
> + tree labels = build_asm_label ("lab");
> +
> + const char *templ = "%[out] %w[out] %cc[in] %l[lab]";
> + tree string = build_string (strlen (templ), templ);
> + tree result = resolve_asm_operand_names (string, outputs, inputs, labels);
> + ASSERT_STREQ ("%0 %w0 %cc1 %l2", TREE_STRING_POINTER (result));
> +
> + templ = "%[fmt]0 %[fmt]1";
> + string = build_string (strlen (templ), templ);
> + result = resolve_asm_operand_names (string, outputs, inputs, labels);
> + ASSERT_STREQ ("%[fmt]0 %[fmt]1", TREE_STRING_POINTER (result));
> +
> + templ = "%[10] %[fmt:0] %[fmt:1]";
> + string = build_string (strlen (templ), templ);
> + result = resolve_asm_operand_names (string, outputs, inputs, labels);
> + ASSERT_STREQ ("%[10] %[fmt:0] %[fmt:1]", TREE_STRING_POINTER (result));
> +
> + templ = "%[fmt:out] %[fmt:in]";
> + string = build_string (strlen (templ), templ);
> + result = resolve_asm_operand_names (string, outputs, inputs, labels);
> + ASSERT_STREQ ("%[fmt:0] %[fmt:1]", TREE_STRING_POINTER (result));
> +}
>
> - return p;
> +void
> +stmt_cc_tests ()
> +{
> + test_resolve_asm_operand_names ();
> }
> +
> +} // namespace selftest
> +
> +#endif /* CHECKING_P */
>
>
> /* Generate RTL to return directly from the current function.
