[ARM] NEON DImode immediate constants

On 28/02/12 16:20, Andrew Stubbs wrote:
> Hi all,
>
> This patch implements 64-bit immediate constant loads in NEON.
>
> The current state is that you can load const_vector, but not const_int.
> This is clearly not ideal. The result is a constant pool entry when it's
> not necessary.
>
> The patch disables the movdi_vfp patterns for loading DImode values, if
> the operand is const_int and NEON is enabled, and extends the neon_mov
> pattern to include DImode const_int, as well as the const_vector
> operands. I've modified neon_valid_immediate only enough to accept
> const_int input - the logic remains untouched.

That patch failed to bootstrap successfully, but this updated patch 
bootstraps and tests with no regressions.

OK?

Andrew

Ping.

On 30/03/12 12:15, Andrew Stubbs wrote:
> On 28/02/12 16:20, Andrew Stubbs wrote:
>> Hi all,
>>
>> This patch implements 64-bit immediate constant loads in NEON.
>>
>> The current state is that you can load const_vector, but not const_int.
>> This is clearly not ideal. The result is a constant pool entry when it's
>> not necessary.
>>
>> The patch disables the movdi_vfp patterns for loading DImode values, if
>> the operand is const_int and NEON is enabled, and extends the neon_mov
>> pattern to include DImode const_int, as well as the const_vector
>> operands. I've modified neon_valid_immediate only enough to accept
>> const_int input - the logic remains untouched.
>
> That patch failed to bootstrap successfully, but this updated patch
> bootstraps and tests with no regressions.
>
> OK?
>
> Andrew

Ping.

On 10/04/12 14:00, Andrew Stubbs wrote:
> Ping.
>
> On 30/03/12 12:15, Andrew Stubbs wrote:
>> On 28/02/12 16:20, Andrew Stubbs wrote:
>>> Hi all,
>>>
>>> This patch implements 64-bit immediate constant loads in NEON.
>>>
>>> The current state is that you can load const_vector, but not const_int.
>>> This is clearly not ideal. The result is a constant pool entry when it's
>>> not necessary.
>>>
>>> The patch disables the movdi_vfp patterns for loading DImode values, if
>>> the operand is const_int and NEON is enabled, and extends the neon_mov
>>> pattern to include DImode const_int, as well as the const_vector
>>> operands. I've modified neon_valid_immediate only enough to accept
>>> const_int input - the logic remains untouched.
>>
>> That patch failed to bootstrap successfully, but this updated patch
>> bootstraps and tests with no regressions.
>>
>> OK?
>>
>> Andrew
>
>

On 30/03/12 12:15, Andrew Stubbs wrote:
> On 28/02/12 16:20, Andrew Stubbs wrote:
>> Hi all,
>>
>> This patch implements 64-bit immediate constant loads in NEON.
>>
>> The current state is that you can load const_vector, but not const_int.
>> This is clearly not ideal. The result is a constant pool entry when it's
>> not necessary.
>>
>> The patch disables the movdi_vfp patterns for loading DImode values, if
>> the operand is const_int and NEON is enabled, and extends the neon_mov
>> pattern to include DImode const_int, as well as the const_vector
>> operands. I've modified neon_valid_immediate only enough to accept
>> const_int input - the logic remains untouched.
> 
> That patch failed to bootstrap successfully, but this updated patch 
> bootstraps and tests with no regressions.
> 
> OK?

Sorry for the delay.

This is OK.

It would be good to merge all the target32 movdi variants into one
pattern and then use alternative enabling to deal with the different
valid alternatives.

R.
> 
> Andrew
> 
> 
> neon-loadimm64.patch
> 
> 
> 2012-03-27  Andrew Stubbs  <ams@codesourcery.com>
> 
> 	gcc/
> 	* config/arm/arm.c (neon_valid_immediate): Allow const_int.
> 	(arm_print_operand): Add 'x' format.
> 	* config/arm/constraints.md (Dn): Allow const_int.
> 	* config/arm/neon.md (neon_mov<mode>): Use VDX to allow DImode.
> 	Use 'x' format to print constants.
> 	* config/arm/predicates.md (imm_for_neon_mov_operand): Allow const_int.
> 	* config/arm/vfp.md (movdi_vfp): Disable for const_int when neon
> 	is enabled.
> 	(movdi_vfp_cortexa8): Likewise.
> 
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 0bded8d..492ddde 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -8873,11 +8873,25 @@ neon_valid_immediate (rtx op, enum machine_mode mode, int inverse,
>        break;					\
>      }
>  
> -  unsigned int i, elsize = 0, idx = 0, n_elts = CONST_VECTOR_NUNITS (op);
> -  unsigned int innersize = GET_MODE_SIZE (GET_MODE_INNER (mode));
> +  unsigned int i, elsize = 0, idx = 0, n_elts;
> +  unsigned int innersize;
>    unsigned char bytes[16];
>    int immtype = -1, matches;
>    unsigned int invmask = inverse ? 0xff : 0;
> +  bool vector = GET_CODE (op) == CONST_VECTOR;
> +
> +  if (vector)
> +    {
> +      n_elts = CONST_VECTOR_NUNITS (op);
> +      innersize = GET_MODE_SIZE (GET_MODE_INNER (mode));
> +    }
> +  else
> +    {
> +      n_elts = 1;
> +      if (mode == VOIDmode)
> +	mode = DImode;
> +      innersize = GET_MODE_SIZE (mode);
> +    }
>  
>    /* Vectors of float constants.  */
>    if (GET_MODE_CLASS (mode) == MODE_VECTOR_FLOAT)
> @@ -8913,7 +8927,7 @@ neon_valid_immediate (rtx op, enum machine_mode mode, int inverse,
>    /* Splat vector constant out into a byte vector.  */
>    for (i = 0; i < n_elts; i++)
>      {
> -      rtx el = CONST_VECTOR_ELT (op, i);
> +      rtx el = vector ? CONST_VECTOR_ELT (op, i) : op;
>        unsigned HOST_WIDE_INT elpart;
>        unsigned int part, parts;
>  
> @@ -17230,6 +17244,19 @@ arm_print_operand (FILE *stream, rtx x, int code)
>  	}
>        return;
>  
> +    /* An integer that we want to print in HEX.  */
> +    case 'x':
> +      switch (GET_CODE (x))
> +	{
> +	case CONST_INT:
> +	  fprintf (stream, "#" HOST_WIDE_INT_PRINT_HEX, INTVAL (x));
> +	  break;
> +
> +	default:
> +	  output_operand_lossage ("Unsupported operand for code '%c'", code);
> +	}
> +      return;
> +
>      case 'B':
>        if (GET_CODE (x) == CONST_INT)
>  	{
> diff --git a/gcc/config/arm/constraints.md b/gcc/config/arm/constraints.md
> index 7d0269a..68979c1 100644
> --- a/gcc/config/arm/constraints.md
> +++ b/gcc/config/arm/constraints.md
> @@ -255,9 +255,9 @@
>  
>  (define_constraint "Dn"
>   "@internal
> -  In ARM/Thumb-2 state a const_vector which can be loaded with a Neon vmov
> -  immediate instruction."
> - (and (match_code "const_vector")
> +  In ARM/Thumb-2 state a const_vector or const_int which can be loaded with a
> +  Neon vmov immediate instruction."
> + (and (match_code "const_vector,const_int")
>        (match_test "TARGET_32BIT
>  		   && imm_for_neon_mov_operand (op, GET_MODE (op))")))
>  
> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
> index d7caa37..3c88568 100644
> --- a/gcc/config/arm/neon.md
> +++ b/gcc/config/arm/neon.md
> @@ -152,9 +152,9 @@
>  (define_attr "vqh_mnem" "vadd,vmin,vmax" (const_string "vadd"))
>  
>  (define_insn "*neon_mov<mode>"
> -  [(set (match_operand:VD 0 "nonimmediate_operand"
> +  [(set (match_operand:VDX 0 "nonimmediate_operand"
>  	  "=w,Uv,w, w,  ?r,?w,?r,?r, ?Us")
> -	(match_operand:VD 1 "general_operand"
> +	(match_operand:VDX 1 "general_operand"
>  	  " w,w, Dn,Uvi, w, r, r, Usi,r"))]
>    "TARGET_NEON
>     && (register_operand (operands[0], <MODE>mode)
> @@ -173,7 +173,7 @@
>        if (width == 0)
>          return "vmov.f32\t%P0, %1  @ <mode>";
>        else
> -        sprintf (templ, "vmov.i%d\t%%P0, %%1  @ <mode>", width);
> +        sprintf (templ, "vmov.i%d\t%%P0, %%x1  @ <mode>", width);
>  
>        return templ;
>      }
> diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md
> index b535335..8a8a1f1 100644
> --- a/gcc/config/arm/predicates.md
> +++ b/gcc/config/arm/predicates.md
> @@ -630,7 +630,7 @@
>  })
>  
>  (define_predicate "imm_for_neon_mov_operand"
> -  (match_code "const_vector")
> +  (match_code "const_vector,const_int")
>  {
>    return neon_immediate_valid_for_move (op, mode, NULL, NULL);
>  })
> diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md
> index 6530570..2061414 100644
> --- a/gcc/config/arm/vfp.md
> +++ b/gcc/config/arm/vfp.md
> @@ -138,7 +138,9 @@
>         (match_operand:DI 1 "di_operand"              "r,rDa,Db,Dc,mi,mi,r,r,w,w,Uvi,w"))]
>    "TARGET_32BIT && TARGET_HARD_FLOAT && TARGET_VFP && arm_tune != cortexa8
>     && (   register_operand (operands[0], DImode)
> -       || register_operand (operands[1], DImode))"
> +       || register_operand (operands[1], DImode))
> +   && !(TARGET_NEON && CONST_INT_P (operands[1])
> +        && neon_immediate_valid_for_move (operands[1], DImode, NULL, NULL))"
>    "*
>    switch (which_alternative)
>      {
> @@ -187,7 +189,9 @@
>         (match_operand:DI 1 "di_operand"              "r,rDa,Db,Dc,mi,mi,r,r,w,w,Uvi,w"))]
>    "TARGET_32BIT && TARGET_HARD_FLOAT && TARGET_VFP && arm_tune == cortexa8
>      && (   register_operand (operands[0], DImode)
> -        || register_operand (operands[1], DImode))"
> +        || register_operand (operands[1], DImode))
> +    && !(TARGET_NEON && CONST_INT_P (operands[1])
> +	 && neon_immediate_valid_for_move (operands[1], DImode, NULL, NULL))"
>    "*
>    switch (which_alternative)
>      {

On 27/04/12 16:17, Richard Earnshaw wrote:
> This is OK.

Thanks, now committed.

> It would be good to merge all the target32 movdi variants into one
> pattern and then use alternative enabling to deal with the different
> valid alternatives.

Yes, I'll take a look.

Andrew