Re: PING: [11/nn] [AArch64] Set NUM_POLY_INT_COEFFS to 2

[James Greenhalgh <james.greenhalgh@arm.com>]

On Fri, Jan 05, 2018 at 11:26:59AM +0000, Richard Sandiford wrote:
> Ping.  Here's the patch updated to apply on top of the v8.4 and
> __builtin_load_no_speculate support.
> 
> Richard Sandiford <richard.sandiford@linaro.org> writes:
> > This patch switches the AArch64 port to use 2 poly_int coefficients
> > and updates code as necessary to keep it compiling.
> >
> > One potentially-significant change is to
> > aarch64_hard_regno_caller_save_mode.  The old implementation
> > was written in a pretty conservative way: it changed the default
> > behaviour for single-register values, but used the default handling
> > for multi-register values.
> >
> > I don't think that's necessary, since the interesting cases for this
> > macro are usually the single-register ones.  Multi-register modes take
> > up the whole of the constituent registers and the move patterns for all
> > multi-register modes should be equally good.
> >
> > Using the original mode for multi-register cases stops us from using
> > SVE modes to spill multi-register NEON values.  This was caught by
> > gcc.c-torture/execute/pr47538.c.
> >
> > Also, aarch64_shift_truncation_mask used GET_MODE_BITSIZE - 1.
> > GET_MODE_UNIT_BITSIZE - 1 is equivalent for the cases that it handles
> > (which are all scalars), and I think it's more obvious, since if we ever
> > do use this for elementwise shifts of vector modes, the mask will depend
> > on the number of bits in each element rather than the number of bits in
> > the whole vector.

This is OK for trunk, with whatever modifications are needed to make the
rebase work. I do have one question and one minor comment; you can assume
that if you do make modifications in response to these that the patch is
still OK.

> Index: gcc/config/aarch64/aarch64.c
> ===================================================================
> --- gcc/config/aarch64/aarch64.c	2018-01-05 11:24:44.647408566 +0000
> +++ gcc/config/aarch64/aarch64.c	2018-01-05 11:24:44.867399574 +0000
> @@ -2262,8 +2258,12 @@ aarch64_pass_by_reference (cumulative_ar
>    int nregs;
>  
>    /* GET_MODE_SIZE (BLKmode) is useless since it is 0.  */
> -  size = (mode == BLKmode && type)
> -    ? int_size_in_bytes (type) : (int) GET_MODE_SIZE (mode);
> +  if (mode == BLKmode && type)
> +    size = int_size_in_bytes (type);
> +  else
> +    /* No frontends can create types with variable-sized modes, so we
> +       shouldn't be asked to pass or return them.  */
> +    size = GET_MODE_SIZE (mode).to_constant ();

I presume that the assertion in your comment is checked in the
GET_MODE_SIZE (mode).to_constant (); call? If not, is it worth making the
assert explicit here?

> @@ -11874,8 +11921,9 @@ aarch64_simd_valid_immediate (rtx op, si
>    unsigned int n_elts;
>    if (const_vec_duplicate_p (op, &elt))
>      n_elts = 1;
> -  else if (GET_CODE (op) == CONST_VECTOR)
> -    n_elts = CONST_VECTOR_NUNITS (op);
> +  else if (GET_CODE (op) == CONST_VECTOR
> +	   && CONST_VECTOR_NUNITS (op).is_constant (&n_elts))
> +    ;
>    else
>      return false;
>  

A comment in the empty else if case would be useful for clarity.

Thanks,
James