[GCC][PATCH][Aarch64] Add Bfloat16_t scalar type, vector types and machine modes to Aarch64 back-end [2/2]

[Richard Sandiford]

Stam Markianos-Wright <Stam.Markianos-Wright@arm.com> writes:
> On 12/19/19 10:08 AM, Richard Sandiford wrote:
>> Stam Markianos-Wright <Stam.Markianos-Wright@arm.com> writes:
>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>>> index f57469b6e23..f40f6432fd4 100644
>>> --- a/gcc/config/aarch64/aarch64.c
>>> +++ b/gcc/config/aarch64/aarch64.c
>>> @@ -21661,6 +21661,68 @@ aarch64_stack_protect_guard (void)
>>>     return NULL_TREE;
>>>   }
>>>   
>>> +/* Return the diagnostic message string if conversion from FROMTYPE to
>>> +   TOTYPE is not allowed, NULL otherwise.  */
>>> +
>>> +static const char *
>>> +aarch64_invalid_conversion (const_tree fromtype, const_tree totype)
>>> +{
>>> +  static char templ[100];
>>> +  if ((GET_MODE_INNER (TYPE_MODE (fromtype)) == BFmode
>>> +       || GET_MODE_INNER (TYPE_MODE (totype)) == BFmode)
>>> +       && TYPE_MODE (fromtype) != TYPE_MODE (totype))
>>> +  {
>>> +    snprintf (templ, sizeof (templ), \
>>> +      "incompatible types when assigning to type '%s' from type '%s'",
>>> +      IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (totype))),
>>> +      IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (fromtype))));
>>> +    return N_(templ);
>>> +  }
>>> +  /* Conversion allowed.  */
>>> +  return NULL;
>>> +}
>>> +
>> 
>> This won't handle translation properly.  We also have no guarantee that
>> the formatted string will fit in 100 characters since at least one of
>> the type names is unconstrained.  (Also, not all types have names.)
>> 
>
> Hi Richard. I'm sending an email here to show you what I have done here, too :)
>
> Currently I have the following:
>
> static const char *
> aarch64_invalid_conversion (const_tree fromtype, const_tree totype)
> {
>    static char templ[100];
>    if (TYPE_MODE (fromtype) != TYPE_MODE (totype)
>        && ((TYPE_MODE (fromtype) == BFmode && !VECTOR_TYPE_P (fromtype))
> 	  || (TYPE_MODE (totype) == BFmode && !VECTOR_TYPE_P (totype))))

Just:

    if (TYPE_MODE (fromtype) != TYPE_MODE (totype)
        && (TYPE_MODE (fromtype) == BFmode || TYPE_MODE (fromtype) == BFmode))

should be enough.  Types that have BFmode can't also be vectors.

>      {
>        if (TYPE_NAME (fromtype) != NULL && TYPE_NAME (totype) != NULL)
> 	{
> 	  snprintf (templ, sizeof (templ),
> 	    "incompatible types when assigning to type '%s' from type '%s'",
> 	    IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (totype))),
> 	    IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (fromtype))));
> 	  return N_(templ);
> 	}
>        else
> 	{
> 	  snprintf (templ, sizeof (templ),
> 	    "incompatible types for assignment");
> 	  return N_(templ);
> 	}

This still has the problem I mentioned above though: DECL_NAMEs are
supplied by the user and can be arbitrary lengths, so there's no
guarantee that the error message fits in the 100-character buffer.
We would get a truncated message if the buffer isn't big enough.

As far as translation goes: the arguments to diagnostic functions
like "error" are untranslated strings, which the diagnostic functions
then translate internally.  po/exgettext scans the source tree looking
for strings that need to be translatable and collects them all in po/gcc.pot.
Constant format strings in calls to known diagnostic functions get picked
up automatically (see ABOUT-GCC-NLS), but others need to be marked with
N_().  This N_() is simply a no-op wrapper macro that marks the argument
as needing translation.  It has no effect if the argument isn't a
constant string.

The interface of this hook is to return an untranslated diagnostic string
that gets passed to error.  A better interface would be to let the hook
raise its own error and return a boolean result, but that isn't what
we have.

So in the above, it's "incompatible types for assignment" that needs to
be wrapped in N_().  Wrapping templ has no effect.

This is also why the first arm doesn't work for translation.  It constructs
and returns an arbitrary new string that won't have been entered into
gcc.pot (and can't be, since it depends on the names of the user types).
So the error function will have no chance to translate it.  And it would
be a layering violation to try to translate it here.

So the hook basically has to return fixed strings marked with N_().
I don't think it should mention assignment though, since the conversions
could occur in any context (initialisation, function calls, etc.).  If
"invalid conversion" seems too terse, maybe we could have things like:

  "invalid conversion to %<bfloat16_t%>"

and:

  "invalid conversion to %<bfloat16_t%>"

>      }
>    /* Conversion allowed.  */
>    return NULL;
> }
>
> This blocks the conversion only if the two types are of different modes and one 
> of them is a BFmode scalar.
>
> Doing it like this seems to block all scalar-sized assignments:
>
> C:
>
> typedef bfloat16_t vbf __attribute__((vector_size(2)));
> vbf foo3 (void) { return (vbf) 0x1234; }
>
> bfloat16_t foo1 (void) { return (bfloat16_t) 0x1234; }
>
> bfloat16_t scalar1_3 = 0;
> bfloat16_t scalar1_4 = 0.1;
> bfloat16_t scalar1_5 = is_a_float;
>
> bfloat16x4_t vector2_8 = { 0.0, 0, n2, is_a_float }; // (blocked on each element 
> assignment)
>
>
> C++:
>
> bfloat16_t c1 (void) { return bfloat16_t (0x1234); }
>
> bfloat16_t c2 (void) { return bfloat16_t (0.1); }
>
>
> But then it allows vector initialisation from binary:
>
> C:
> bfloat16x4_t foo1 (void) { return (bfloat16x4_t) 0x1234567812345678; }
>
> C++:
> bfloat16x4_t foo1 (void) { return bfloat16x4_t (0x1234567812345678); }
> typedef bfloat16_t v2bf __attribute__((vector_size(4)));
> v2bf foo3 (void) { return v2bf (0x12345678); }
>
> I also need to check with a colleague who is on holiday if any of this impacts 
> the vector-reinterpret intrinsics that he was working on...
>
> Let me know of your thoughts!

Sounds good to me.  I hadn't realised when talking about the "(vector) int"
thing that this hook would block it, and TBH it probably isn't important
enough to go out of our way to unblock it.  But I think this is also the
behaviour we want for vector-vector conversions, which definitely are
important.

Thanks,
Richard