Re: [Patch 2/4][Aarch64] v2: Implement Aarch64 SIMD ABI

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

Steve Ellcey <sellcey@marvell.com> writes:
> On Fri, 2019-01-11 at 14:45 +0000, Richard Sandiford wrote:
>> 
>> > +
>> > +/* Return true for types that could be supported as SIMD return or
>> > +   argument types.  */
>> > +
>> > +static bool supported_simd_type (tree t)
>> > +{
>> > +  return (FLOAT_TYPE_P (t) || INTEGRAL_TYPE_P (t));
>> 
>> We should also check that the size is 1, 2, 4 or 8 bytes.
>
> I fixed this, I also allow for POINTER_P types which allowed me
> to not do the POINTER_P check below which you asked about and
> which I now think was a mistake (more comments below).

Ah, yeah, agree that's the right thing to do.

>> > +  if (clonei->simdlen == 0)
>> > +    {
>> > +      if (SCALAR_INT_MODE_P (TYPE_MODE (base_type)))
>> > +	clonei->simdlen = clonei->vecsize_int;
>> > +      else
>> > +	clonei->simdlen = clonei->vecsize_float;
>> > +      clonei->simdlen /= GET_MODE_BITSIZE (SCALAR_TYPE_MODE (base_type));
>> > +      return 1;
>> > +    }
>> 
>> I should have noticed this last time, but base_type is the CDT in the
>> Intel ABI.  That isn't always right for the AArch64 ABI.
>> 
>> I think for now currently_supported_simd_type should take base_type
>> as a second parameter and check that the given type has the same
>> size.
>
> I have not changed this, I am not quite sure what you mean.  What is
> CDT?  Clone data type?  Are you saying I should use node->decl->type
> instead of base_type?

CDT is the Characteristic Data Type and is specific to the Intel ABI:

/* Given a SIMD clone in NODE, calculate the characteristic data
   type and return the coresponding type.  The characteristic data
   type is computed as described in the Intel Vector ABI.  */

static tree
simd_clone_compute_base_data_type (struct cgraph_node *node,
				   struct cgraph_simd_clone *clone_info)

This has consequences that we didn't want for AArch64, such as
assigning different implicit simdlens for "double f(float)" and
"float g(double)".  The rules also don't extend naturally to
SVE-like architectures, where mixed data sizes are best handled
using unpacked vectors.

But at this stage I think it would be better to leave cases in which
the Intel ABI gives a different mapping from the AArch64 ABI to GCC 10.
For GCC 9 it seems better to handle only the cases that are the same
under both ABIs.

And using the CDT is stil OK in the trivial case that the return type
and arguments are all supported vector element types and all have the
same size.  So I think for GCC 9 we should just handle that case.

We can do that by passing base_type as a second argument to
currently_supported_simd_type and checking that the first argument
has the same size.

> @@ -18420,6 +18422,140 @@ aarch64_estimated_poly_value (poly_int64 val)
>    return val.coeffs[0] + val.coeffs[1] * over_128 / 128;
>  }
>  
> +
> +/* Return true for types that could be supported as SIMD return or
> +   argument types.  */
> +
> +static bool supported_simd_type (tree t)
> +{
> +  HOST_WIDE_INT s;
> +  gcc_assert (tree_fits_shwi_p (TYPE_SIZE_UNIT (t)));
> +  s = tree_to_shwi (TYPE_SIZE_UNIT (t));
> +  return ((FLOAT_TYPE_P (t) || INTEGRAL_TYPE_P (t) || POINTER_TYPE_P (t))
> +	  && (s == 1 || s == 2 || s == 4 || s == 8));

We should only assert after checking FLOAT_TYPE_P etc.  And there's
no need to assert explicitly, since tree_to_shwi already asserts
where necessary.  So I think this should be:

  if (SCALAR_FLOAT_TYPE_P (t) || INTEGRAL_TYPE_P (t) || POINTER_TYPE_P (t))
    {
      HOST_WIDE_INT s = tree_to_shwi (TYPE_SIZE_UNIT (t));
      return s == 1 || s == 2 || s == 4 || s == 8;
    }
  return false;
  
(SCALAR_FLOAT_TYPE_P so that the tests are consistent about not handling
complex types, sorry for not thinking about that before.)

> +  clonei->vecsize_mangle = 'n';
> +  clonei->mask_mode = VOIDmode;
> +  clonei->vecsize_int = 128;
> +  clonei->vecsize_float = 128;
> +
> +  if (clonei->simdlen == 0)
> +    {
> +      if (SCALAR_INT_MODE_P (TYPE_MODE (base_type)))
> +	clonei->simdlen = clonei->vecsize_int;
> +      else
> +	clonei->simdlen = clonei->vecsize_float;
> +      clonei->simdlen /= GET_MODE_BITSIZE (SCALAR_TYPE_MODE (base_type));
> +      return 1;

The AArch64 vector ABI says that having no simdlen should imply both
64-bit and 128-bit implementations.  E.g.:

   #pragma omp declare simd
   int32_t foo(int32_t x);

should provide:

   int32x2_t _ZGVnN2v_foo(int32x2_t vx);
   int32x2_t _ZGVnM2v_foo(int32x2_t vx, uint32x2_t vmask);
   int32x4_t _ZGVnN4v_foo(int32x4_t vx);
   int32x4_t _ZGVnM4v_foo(int32x4_t vx, uint32x4_t vmask);

whereas here we're only providing the 128-bit versions.  I think we should:

- return 2
- set vecsize_int and vecsize_float to 64 when num==0
- set vecsize_int and vecsize_float to 128 when num==1

> +  /* Restrict ourselves to vectors that fit in a single register  */
> +
> +  gcc_assert (tree_fits_shwi_p (TYPE_SIZE (base_type)));
> +  vsize = clonei->simdlen * tree_to_shwi (TYPE_SIZE (base_type));
> +  if (vsize > 128)
> +    {
> +      warning_at (DECL_SOURCE_LOCATION (node->decl), 0,
> +		  "GCC does not currently support simdlen %d for type %qT",
> +		  clonei->simdlen, base_type);
> +      return 0;
> +    }
> +  return 1;

This only really handles cases in which vsize is exactly 128,
because it doesn't update vecsize_int or vecsize_float.

I think we should allow only 64 and 128 (for now) and set vecsize_int
and vecsize_float to vsize.

> +}
> +
> +/* Implement TARGET_SIMD_CLONE_ADJUST.  */
> +
> +static void
> +aarch64_simd_clone_adjust (struct cgraph_node *node)
> +{
> +  /* Add aarch64_vector_pcs target attribute to SIMD clones so they
> +     use the correct ABI.  */
> +
> +  tree t = TREE_TYPE (node->decl);
> +  TYPE_ATTRIBUTES (t) =
> +    make_attribute ("aarch64_vector_pcs", "default", TYPE_ATTRIBUTES (t));

Formatting nit: "=" shouldn't be at the end of a line.  Maybe:

  TYPE_ATTRIBUTES (t) = make_attribute ("aarch64_vector_pcs", "default",
					TYPE_ATTRIBUTES (t));

Thanks,
Richard