[PATCH V2] aarch64: Don't include vec_select in SIMD multiply cost

Richard Sandiford richard.sandiford@arm.com
Wed Aug 4 08:51:35 GMT 2021


Jonathan Wright via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Hi,
>
> V2 of the patch addresses the initial review comments, factors out
> common code (as we discussed off-list) and adds a set of unit tests
> to verify the code generation benefit.
>
> Regression tested and bootstrapped on aarch64-none-linux-gnu - no
> issues.
>
> Ok for master?
>
> Thanks,
> Jonathan
>
> ---
>
> gcc/ChangeLog:
>
> 2021-07-19  Jonathan Wright  <jonathan.wright@arm.com>
>
> 	* config/aarch64/aarch64.c (aarch64_strip_duplicate_vec_elt):
> 	Define.
> 	(aarch64_rtx_mult_cost): Traverse RTL tree to prevent
> 	vec_select cost from being added into Neon multiply cost.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/vmul_element_cost.c: New test.
>
>
>
> From: Richard Sandiford <richard.sandiford@arm.com>
> Sent: 22 July 2021 18:16
> To: Jonathan Wright <Jonathan.Wright@arm.com>
> Cc: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> Subject: Re: [PATCH] aarch64: Don't include vec_select in SIMD multiply cost 
>  
> Jonathan Wright <Jonathan.Wright@arm.com> writes:
>> Hi,
>>
>> The Neon multiply/multiply-accumulate/multiply-subtract instructions
>> can take various forms - multiplying full vector registers of values
>> or multiplying one vector by a single element of another. Regardless
>> of the form used, these instructions have the same cost, and this
>> should be reflected by the RTL cost function.
>>
>> This patch adds RTL tree traversal in the Neon multiply cost function
>> to match the vec_select used by the lane-referencing forms of the
>> instructions already mentioned. This traversal prevents the cost of
>> the vec_select from being added into the cost of the multiply -
>> meaning that these instructions can now be emitted in the combine
>> pass as they are no longer deemed prohibitively expensive.
>>
>> Regression tested and bootstrapped on aarch64-none-linux-gnu - no
>> issues.
>>
>> Ok for master?
>>
>> Thanks,
>> Jonathan
>>
>> ---
>>
>> gcc/ChangeLog:
>>
>> 2021-07-19  Jonathan Wright  <jonathan.wright@arm.com>
>>
>>         * config/aarch64/aarch64.c (aarch64_rtx_mult_cost): Traverse
>>         RTL tree to prevents vec_select from being added into Neon
>>         multiply cost.
>>
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index f5b25a7f7041645921e6ad85714efda73b993492..b368303b0e699229266e6d008e28179c496bf8cd 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -11985,6 +11985,21 @@ aarch64_rtx_mult_cost (rtx x, enum rtx_code code, int outer, bool speed)
>>            op0 = XEXP (op0, 0);
>>          else if (GET_CODE (op1) == VEC_DUPLICATE)
>>            op1 = XEXP (op1, 0);
>> +       /* The same argument applies to the VEC_SELECT when using the lane-
>> +          referencing forms of the MUL/MLA/MLS instructions. Without the
>> +          traversal here, the combine pass deems these patterns too
>> +          expensive and subsequently does not emit the lane-referencing
>> +          forms of the instructions. In addition, canonical form is for the
>> +          VEC_SELECT to be the second argument of the multiply - thus only
>> +          op1 is traversed.  */
>> +       if (GET_CODE (op1) == VEC_SELECT
>> +           && GET_MODE_NUNITS (GET_MODE (op1)).to_constant () == 1)
>> +         op1 = XEXP (op1, 0);
>> +       else if ((GET_CODE (op1) == ZERO_EXTEND
>> +                 || GET_CODE (op1) == SIGN_EXTEND)
>> +                && GET_CODE (XEXP (op1, 0)) == VEC_SELECT
>> +                && GET_MODE_NUNITS (GET_MODE (op1)).to_constant () == 1)
>> +         op1 = XEXP (XEXP (op1, 0), 0);
>
> I think this logically belongs in the “GET_CODE (op1) == VEC_DUPLICATE”
> if block, since the condition is never true otherwise.  We can probably
> skip the GET_MODE_NUNITS tests, but if you'd prefer to keep them, I think
> it would be better to add them to the existing VEC_DUPLICATE tests rather
> than restrict them to the VEC_SELECT ones.
>
> Also, although this is in Advanced SIMD-specific code, I think it'd be
> better to use:
>
>   is_a<scalar_mode> (GET_MODE (op1))
>
> instead of:
>
>   GET_MODE_NUNITS (GET_MODE (op1)).to_constant () == 1
>
> Do you have a testcase?
>
> Thanks,
> Richard
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 3bdf19d71b54d0ade8e5648323f6e1f012bc4f8f..5809887997305317c5a81421089db431685e2927 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -11908,6 +11908,26 @@ aarch64_strip_extend (rtx x, bool strip_shift)
>    return x;
>  }
>  
> +
> +/* Helper function for rtx cost calculation. Strip VEC_DUPLICATE as well as
> +   any subsequent extend and VEC_SELECT from X. Returns the inner scalar
> +   operand if successful, or the original expression on failure.  */
> +static rtx
> +aarch64_strip_duplicate_vec_elt (rtx x)
> +{
> +  if (GET_CODE (x) == VEC_DUPLICATE
> +      && is_a<scalar_mode> (GET_MODE (XEXP (x, 0))))
> +    {
> +      x = XEXP (x, 0);
> +      if (GET_CODE (x) == VEC_SELECT)
> +	x = XEXP (x, 0);
> +      else if ((GET_CODE (x) == ZERO_EXTEND || GET_CODE (x) == SIGN_EXTEND)
> +	       && GET_CODE (XEXP (x, 0)) == VEC_SELECT)
> +	x = XEXP (XEXP (x, 0), 0);
> +    }
> +  return x;
> +}
> +
>  /* Return true iff CODE is a shift supported in combination
>     with arithmetic instructions.  */
>  
> @@ -11977,14 +11997,14 @@ aarch64_rtx_mult_cost (rtx x, enum rtx_code code, int outer, bool speed)
>  	{
>  	  /* The by-element versions of the instruction have the same costs as
>  	     the normal 3-vector version.  So don't add the costs of the
> -	     duplicate into the costs of the multiply.  We make an assumption
> -	     that the input to the VEC_DUPLICATE is already on the FP & SIMD
> -	     side.  This means costing of a MUL by element pre RA is a bit
> -	     optimistic.  */
> +	     duplicate or subsequent select into the costs of the multiply.  We

Very pedantic, but: the select conceptually happens before the duplicate.
TBH I think we can probably just drop this sentence, since the calls
make the operation self-description.  (The other parts of the comment
are still useful.)

> +	     make an assumption that the input to the VEC_DUPLICATE is already
> +	     on the FP & SIMD side.  This means costing of a MUL by element pre
> +	     RA is a bit optimistic.  */
>  	  if (GET_CODE (op0) == VEC_DUPLICATE)
> -	    op0 = XEXP (op0, 0);
> +	    op0 = aarch64_strip_duplicate_vec_elt (op0);
>  	  else if (GET_CODE (op1) == VEC_DUPLICATE)
> -	    op1 = XEXP (op1, 0);
> +	    op1 = aarch64_strip_duplicate_vec_elt (op1);

I think we might as well call aarch64_strip_duplicate_vec_elt
unconditionally, without the VEC_DUPLICATE tests.

OK with those changes, and sorry for the slow review.

Thanks,
Richard

>  	}
>        cost += rtx_cost (op0, mode, MULT, 0, speed);
>        cost += rtx_cost (op1, mode, MULT, 1, speed);
> diff --git a/gcc/testsuite/gcc.target/aarch64/vmul_element_cost.c b/gcc/testsuite/gcc.target/aarch64/vmul_element_cost.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..c153775f0914072fb985b18516f110aded7dccd5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/vmul_element_cost.c
> @@ -0,0 +1,94 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3" } */
> +
> +#include <arm_neon.h>
> +
> +#define TEST_MUL_UNIFORM(name, q, vectype, ts) \
> +  vectype test_ ## name ## q ## _ ## ts (vectype a, vectype b, vectype c) \
> +	{ \
> +		vectype t0 = name ## q ## _n_ ## ts (a, c[1]); \
> +		vectype t1 = name ## q ## _n_ ## ts (b, c[1]); \
> +		return vmul ## q ## _ ## ts (t0, t1); \
> +	}
> +
> +TEST_MUL_UNIFORM (vmul, , int16x4_t, s16)
> +TEST_MUL_UNIFORM (vmul, , uint16x4_t, u16)
> +TEST_MUL_UNIFORM (vmul, , int32x2_t, s32)
> +TEST_MUL_UNIFORM (vmul, , uint32x2_t, u32)
> +TEST_MUL_UNIFORM (vmul, , float32x2_t, f32)
> +TEST_MUL_UNIFORM (vmul, q, int16x8_t, s16)
> +TEST_MUL_UNIFORM (vmul, q, uint16x8_t, u16)
> +TEST_MUL_UNIFORM (vmul, q, int32x4_t, s32)
> +TEST_MUL_UNIFORM (vmul, q, uint32x4_t, u32)
> +TEST_MUL_UNIFORM (vmul, q, float32x4_t, f32)
> +TEST_MUL_UNIFORM (vmul, q, float64x2_t, f64)
> +
> +#define TEST_MLX_UNIFORM(name, q, vectype, ts) \
> +  vectype test_ ## name ## q ## _ ## ts (vectype acc, vectype a, vectype b) \
> +	{ \
> +		acc = name ## q ## _n_ ## ts (acc, a, b[1]); \
> +		return name ## q ## _n_ ## ts (acc, a, b[1]); \
> +	}
> +
> +TEST_MLX_UNIFORM (vmla, , int16x4_t, s16)
> +TEST_MLX_UNIFORM (vmla, , uint16x4_t, u16)
> +TEST_MLX_UNIFORM (vmla, , int32x2_t, s32)
> +TEST_MLX_UNIFORM (vmla, , uint32x2_t, u32)
> +TEST_MLX_UNIFORM (vmla, , float32x2_t, f32)
> +TEST_MLX_UNIFORM (vmla, q, int16x8_t, s16)
> +TEST_MLX_UNIFORM (vmla, q, uint16x8_t, u16)
> +TEST_MLX_UNIFORM (vmla, q, int32x4_t, s32)
> +TEST_MLX_UNIFORM (vmla, q, uint32x4_t, u32)
> +TEST_MLX_UNIFORM (vmla, q, float32x4_t, f32)
> +
> +TEST_MLX_UNIFORM (vmls, , int16x4_t, s16)
> +TEST_MLX_UNIFORM (vmls, , uint16x4_t, u16)
> +TEST_MLX_UNIFORM (vmls, , int32x2_t, s32)
> +TEST_MLX_UNIFORM (vmls, , uint32x2_t, u32)
> +TEST_MLX_UNIFORM (vmls, , float32x2_t, f32)
> +TEST_MLX_UNIFORM (vmls, q, int16x8_t, s16)
> +TEST_MLX_UNIFORM (vmls, q, uint16x8_t, u16)
> +TEST_MLX_UNIFORM (vmls, q, int32x4_t, s32)
> +TEST_MLX_UNIFORM (vmls, q, uint32x4_t, u32)
> +TEST_MLX_UNIFORM (vmls, q, float32x4_t, f32)
> +
> +#define TEST_MUL_LONG(name, rettype, intype, ts, rs) \
> +  rettype test_ ## name ## ts (intype a, intype b, intype c) \
> +	{ \
> +		rettype t0 = name ## ts (a, c[1]); \
> +		rettype t1 = name ## ts (b, c[1]); \
> +		return vqaddq ## _ ## rs (t0, t1); \
> +	}
> +
> +TEST_MUL_LONG (vmull_n_, int32x4_t, int16x4_t, s16, s32)
> +TEST_MUL_LONG (vmull_n_, uint32x4_t, uint16x4_t, u16, u32)
> +TEST_MUL_LONG (vmull_n_, int64x2_t, int32x2_t, s32, s64)
> +TEST_MUL_LONG (vmull_n_, uint64x2_t, uint32x2_t, u32, u64)
> +
> +TEST_MUL_LONG (vqdmull_n_, int32x4_t, int16x4_t, s16, s32)
> +TEST_MUL_LONG (vqdmull_n_, int64x2_t, int32x2_t, s32, s64)
> +
> +#define TEST_MLX_LONG(name, rettype, intype, ts, rs) \
> +  rettype test_ ## name ## _ ## ts (rettype acc, intype a, intype b) \
> +	{ \
> +		acc = name ## ts (acc, a, b[1]); \
> +		return name ## ts (acc, a, b[1]); \
> +	}
> +
> +TEST_MLX_LONG (vmlal_n_, int32x4_t, int16x4_t, s16, s32)
> +TEST_MLX_LONG (vmlal_n_, uint32x4_t, uint16x4_t, u16, u32)
> +TEST_MLX_LONG (vmlal_n_, int64x2_t, int32x2_t, s32, s64)
> +TEST_MLX_LONG (vmlal_n_, uint64x2_t, uint32x2_t, u32, u64)
> +
> +TEST_MLX_LONG (vmlsl_n_, int32x4_t, int16x4_t, s16, s32)
> +TEST_MLX_LONG (vmlsl_n_, uint32x4_t, uint16x4_t, u16, u32)
> +TEST_MLX_LONG (vmlsl_n_, int64x2_t, int32x2_t, s32, s64)
> +TEST_MLX_LONG (vmlsl_n_, uint64x2_t, uint32x2_t, u32, u64)
> +
> +TEST_MLX_LONG (vqdmlal_n_, int32x4_t, int16x4_t, s16, s32)
> +TEST_MLX_LONG (vqdmlal_n_, int64x2_t, int32x2_t, s32, s64)
> +
> +TEST_MLX_LONG (vqdmlsl_n_, int32x4_t, int16x4_t, s16, s32)
> +TEST_MLX_LONG (vqdmlsl_n_, int64x2_t, int32x2_t, s32, s64)
> +
> +/* { dg-final { scan-assembler-not "dup\\t" } } */


More information about the Gcc-patches mailing list