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

[Stam Markianos-Wright <Stam dot Markianos-Wright at arm dot com>]

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))))
     {
       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);
	}
     }
   /* 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!

Cheers,
Stam

> Unfortunately the interface of the current hook doesn't allow for good
> diagnostics.  We'll just have to return a fixed string. >
> Formatting nit: braced block should be indented two spaces more
> than the "if (...)".
> 
> Same comment for the other hooks.

Done. Will be in next revision

> 
>> +/* Return the diagnostic message string if the unary operation OP is
>> +   not permitted on TYPE, NULL otherwise.  */
>> +
>> +static const char *
>> +aarch64_invalid_unary_op (int op, const_tree type)
>> +{
>> +  static char templ[100];
>> +  /* Reject all single-operand operations on BFmode except for &.  */
>> +  if (GET_MODE_INNER (TYPE_MODE (type)) == BFmode && op != ADDR_EXPR)
>> +  {
>> +    snprintf (templ, sizeof (templ),
>> +      "operation not permitted on type '%s'",
>> +      IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (type))));
>> +    return N_(templ);
>> +  }
>> +  /* Operation allowed.  */
>> +  return NULL;
>> +}
> 
> The problem with testing TYPE_MODE is that we'll then miss things
> that don't have a dedicated mode.  E.g. it'd be interesting to
> test what happens for arithmetic on:
> 
>    typedef bfloat16_t v16bf __attribute__((vector_size(32)));
> 
> Probably better to use element_mode instead.

Done. Will be in next revision

> 
>> diff --git a/gcc/testsuite/gcc.target/aarch64/bfloat16_scalar_typecheck.c b/gcc/testsuite/gcc.target/aarch64/bfloat16_scalar_typecheck.c
>> new file mode 100644
>> index 00000000000..6f6a6af9587
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/bfloat16_scalar_typecheck.c
>> @@ -0,0 +1,83 @@
>> +/* { dg-do compile { target { aarch64*-*-* } } } */
>> +/* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } } */
>> +/* { dg-options "-march=armv8.2-a+i8mm" } */
> 
> +bf16 rather than +i8mm.  But using:
> 
> /* { dg-require-effective-target arm_v8_2a_bf16_neon_ok } */
> /* { dg-add-options arm_v8_2a_bf16_neon }  */
> 
> would be better.

Done. Will be in next revision
> 
>> +
>> +#include <arm_neon.h>
>> +
>> +bfloat16_t glob;
>> +float is_a_float;
>> +int n;
>> +
>> +bfloat16_t footest (bfloat16_t scalar0)
>> +{
>> +
>> +  /* Initialisation  */
>> +
>> +  bfloat16_t scalar1 = 0.1; /* { dg-error "incompatible types when assigning to type 'bfloat16_t' from type 'double'" "" {target *-*-*} } */
>> +  bfloat16_t scalar2 = 0;   /* { dg-error "incompatible types when assigning to type 'bfloat16_t' from type 'int'" "" {target *-*-*} } */
>> +  bfloat16_t scalar3 = {}; /* { dg-error "empty scalar initializer" "" {target *-*-*} } */
> 
> Would also be worth testing { scalar0 }, { is_a_float } and { 0.1 }.

Done. Will be in next revision

> 
> (For SVE the tests are divided between sizeless_1.c and gnu_vectors_1.c.
> Most of the cases mentioned here are handled in gnu_vectors_1.c instead.)
> 
>> +
>> +  float16_t initi_a = scalar1; /* { dg-error "incompatible types when assigning to type 'float16_t' from type 'bfloat16_t'" "" {target *-*-*} } */
>> +  float16_t initi_b = { scalar1 }; /* { dg-error "incompatible types when assigning to type 'float16_t' from type 'bfloat16_t'" "" {target *-*-*} } */
>> +
>> +  /* Compound literals.  */
>> +
>> +  (bfloat16_t) {}; /* { dg-error "empty scalar initializer" "" {target *-*-*} } */
>> +  (bfloat16_t) { scalar1 };
> 
> Same here.

Done. Will be in next revision

> 
>> +
>> +  (int) { scalar1 }; /* { dg-error "incompatible types when assigning to type 'int' from type 'bfloat16_t'" "" {target *-*-*} } */
>> +
>> +  /* Casting.  */
>> +
>> +  (void) scalar1;
>> +  (bfloat16_t) scalar1;
> 
> Would be good to have some tests for invalid cases too.

Done. Will be in next revision

> 
>> +
>> +  /* Arrays and Structs.  */
>> +
>> +  typedef bfloat16_t array_type[2];
>> +  extern bfloat16_t extern_array[];
>> +
>> +  bfloat16_t array[2];
>> +  bfloat16_t zero_length_array[0];
>> +  bfloat16_t empty_init_array[] = {};
>> +  typedef bfloat16_t vla_type[n];
>> +
>> +  struct struct1 {
>> +    bfloat16_t a;
>> +  };
>> +
>> +  union union1 {
>> +    bfloat16_t a;
>> +  };
>> +
>> +  /* Assignments.  */
>> +
>> +  n = scalar1; /* { dg-error "incompatible types when assigning to type 'int' from type 'bfloat16_t'" "" {target *-*-*} } */
>> +  is_a_float = scalar1; /* { dg-error "incompatible types when assigning to type 'float' from type 'bfloat16_t'" "" {target *-*-*} } */
>> +  scalar1 = 0; /* { dg-error "incompatible types when assigning to type 'bfloat16_t' from type 'int'" "" {target *-*-*} } */
>> +  scalar1 = 0.1; /* { dg-error "incompatible types when assigning to type 'bfloat16_t' from type 'double'" "" {target *-*-*} } */
>> +  scalar1 = scalar2;
> 
> Would be good to test the other way too: "scalar1 = is_a_float",
> "scalar1 = n".

Done. Will be in next revision

> 
>> +
>> +  /* Addressing and dereferencing.  */
>> +
>> +  bfloat16_t *bfloat_ptr = &scalar1;
>> +  scalar1 = *bfloat_ptr;
>> +
>> +  /* Pointer assignment.  */
>> +
>> +  bfloat16_t *bfloat_ptr2 = bfloat_ptr;
>> +
>> +  /* Single-operand operation.  */
>> +
>> +  scalar1 = !glob; /* { dg-error "operation not permitted on type 'bfloat16_t'" "" {target *-*-*} } */
> 
> Would be good to test "+" and "-" as well -- "!" isn't really typical
> for floats.
> 

Done. Will be in next revision

> Other things worth testing for are:
> 
> - comparisons
> - bfloats used as a condition (e.g. bfloat16 ? a : b)
> - bfloats selected via ?:, including cases where the types don't match
> 

Done. Will be in next revision

>> [...]
>> diff --git a/gcc/testsuite/gcc.target/aarch64/bfloat16_vector_typecheck1.c b/gcc/testsuite/gcc.target/aarch64/bfloat16_vector_typecheck1.c
> 
> Very minor, but local aarch64 style seems to be to use foo_1, foo_2,
> etc. rather than foo, foo1, etc., although things aren't very consistent.

Done. Will be in next revision

> 
> Similar comments for these tests as for the scalar ones.
> 
> It would be good to have C++ tests too.  An extra thing to test there
> is elementwise vector ? vector : vector.

Done. Will be in next revision

> 
> Thanks,
> Richard
>