Tamar Christina <[email protected]> writes:
>> [...]
>> 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]".
>>
>
> Yeah, Agreed, Richard Earnshaw brought up the same thing internally after
> I posted the patch and I arrived at the same conclusion as you. I was on the
> edge about removing it but happy to do so to avoid ambiguity. My original
> reason for keeping it was because it made the line less "funky" when e.g. the
> syntax is used for lanes. Like [%[foo],0] looked easier to read than
> [%[foo:0]]
> but I realize this is completely subjective, and for the aarch64 patches I
> went
> with %[modifier:operand] anyway.
>
> That syntax also lends more naturally to the other extension I want to do
> where
> The modifier can consume multiple operands, so %[foo:0,1].
Ah, interesting.
>> > [...]
>> > @@ -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;
>> > [...]
>> > 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.
>>
>
> Some of the feedback make me wonder since I'm already changing the parsing
> anyway whether I should merge my follow up into this one as well.
>
> What I wanted was to flip the operand and modifier parse order for the new
> syntax, such that the operand RTL are available in the print_operands
> function.
>
> This allows us to simplify printing by not needing iterators to convey
> the content
> of e.g. constant RTL or the types. For instance printing a scalar
> register no longer
> requires us to explicitly use a different modifier for a particular
> register, we could
> infer it from the mode.
Perhaps I'm misunderstanding, but I think this is already possible.
I'd always assumed that it was a policy decision not to do it for AArch64,
rather than a restriction of the infrastructure. The port could have had
a modifier that infers the choice between %b, %h, %s, %d, or %q from the
mode of the register and avoided using %<Vetype>. (%b etc. still seem
valuable, since there are outlying cases where being able to force a
particular register prefix is useful.)
That was before my time though.
For SVE, we mostly embedded the size suffixes in the asm string for
consistency with Adv SIMD, but we did deviate from it for printing
register ranges:
case 0:
if (x == NULL)
{
output_operand_lossage ("missing operand");
return;
}
switch (GET_CODE (x))
{
case CONST_STRING:
{
asm_fprintf (f, "%s", XSTR (x, 0));
break;
}
case REG:
if (aarch64_sve_data_mode_p (GET_MODE (x)))
{
if (REG_NREGS (x) == 1)
asm_fprintf (f, "z%d", REGNO (x) - V0_REGNUM);
else
{
char suffix
= sizetochar (GET_MODE_UNIT_BITSIZE (GET_MODE (x)));
asm_fprintf (f, "{z%d.%c - z%d.%c}",
REGNO (x) - V0_REGNUM, suffix,
END_REGNO (x) - V0_REGNUM - 1, suffix);
}
}
else
asm_fprintf (f, "%s", reg_names [REGNO (x)]);
break;
Maybe that was the wrong decision and we should have added a single
"Z register with suffix" modifier for both single and multiple registers.
(Of course, we do need to print unsuffixed Z registers for MOVPRFX,
and also need to print P registers without an element size suffix in
several contexts.)
Thanks,
Richard
