Re: [PATCH v2 1/1] aarch64: Avoid extra move for two-element vector lane copies [PR123951]

Thanks for the updated patch.

Rohith Kapelli <[email protected]> writes:
> Since r14-3381 the vec_extract/vec_set idiom used by the vcopy*_lane*
> intrinsics is folded to a VEC_PERM_EXPR, so the constant permute
> expander decides what to emit for it.  For a two-element vector the
> result takes one lane from each input, but the patterns that matched it
> tied the result to one particular input, and for half the lane
> combinations that is the wrong one, so the register allocator has to add
> a move.  On big endian arm_neon.h's lane flip puts
> vcopyq_laneq_u64 (a, 1, b, 1) in that half, so it regressed from one
> instruction to two and vect_copy_lane_1.c and vget_set_lane_1.c fail;
> little endian has the same problem on lane 0.
>
> Add two patterns for the two-element modes, each offering a second
> alternative that ties the destination to the other input, so that the
> register allocator can insert into whichever input already occupies the
> destination.  The first handles a lane copy whose source and destination
> lanes are the same; the second handles a lane insert from a scalar,
> which always sits in the low part of its register, so it is restricted
> to the case where the destination is architectural lane 0.  Both sit
> before the corresponding general patterns, which continue to handle
> every other case, including cross-lane copies.
>
> Both lane combinations of vcopy_lane_{s,u,f}32 and
> vcopyq_laneq_{s,u,f}64 now expand to a single INS on both endiannesses.
> The double add/sub merge in addsub_{1,2}.c uses the same two-element
> merge; with the tie left to the allocator its e1 case now inserts into
> lane 0 rather than lane 1, still a single INS, so the expected output is
> updated.
>
> 	PR tree-optimization/123951
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64-simd.md
> 	(*aarch64_simd_vec_set_lane0<mode>): New pattern.
> 	(*aarch64_simd_vec_copy_lane_same<mode>): New pattern.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/pr123951_1.c: New test.
> 	* gcc.target/aarch64/pr123951_2.c: New test.
> 	* gcc.target/aarch64/simd/addsub_1.c: Update e1 lane insert from
> 	d[1] to d[0].
> 	* gcc.target/aarch64/simd/addsub_2.c: Likewise.
>
> Signed-off-by: Rohith Kapelli <[email protected]>
> ---
>  gcc/config/aarch64/aarch64-simd.md            | 68 +++++++++++++++++++
>  gcc/testsuite/gcc.target/aarch64/pr123951_1.c | 41 +++++++++++
>  gcc/testsuite/gcc.target/aarch64/pr123951_2.c | 35 ++++++++++
>  .../gcc.target/aarch64/simd/addsub_1.c        |  2 +-
>  .../gcc.target/aarch64/simd/addsub_2.c        |  2 +-
>  5 files changed, 146 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/pr123951_1.c
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/pr123951_2.c
>
> diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
> index ce51e24da36..62682794bdc 100644
> --- a/gcc/config/aarch64/aarch64-simd.md
> +++ b/gcc/config/aarch64/aarch64-simd.md
> @@ -1330,6 +1330,44 @@
>    [(set_attr "type" "neon_logic<q>")]
>  )
>  
> +;; Lane insert for the two-element modes.  A scalar source always occupies
> +;; the low part of its register, architectural lane 0.  When the inserted
> +;; element is that same lane, the destination can equally be tied to the
> +;; source register and the other lane brought in from operand 3, so offer
> +;; that as a second alternative and let the register allocator pick
> +;; whichever input already occupies the destination.  The remaining
> +;; alternatives are those of the general pattern below, so that a scalar
> +;; from a general register or from memory is unaffected.
> +(define_insn "*aarch64_simd_vec_set_lane0<mode>"
> +  [(set (match_operand:VP_2E 0 "register_operand" "=w,w,w,w")
> +	(vec_merge:VP_2E
> +	    (vec_duplicate:VP_2E
> +	      (match_operand:<VEL> 1 "aarch64_simd_nonimmediate_operand"
> +	       "w,0,?r,Utv"))
> +	    (match_operand:VP_2E 3 "register_operand" "0,w,0,0")
> +	    (match_operand:SI 2 "immediate_operand" "i,i,i,i")))]
> +  "TARGET_SIMD && INTVAL (operands[2]) == (BYTES_BIG_ENDIAN ? 2 : 1)"
> +  {
> +    switch (which_alternative)
> +      {
> +      case 0:
> +	return "ins\t%0.<Vetype>[0], %1.<Vetype>[0]";
> +      case 1:
> +	/* The destination is the scalar's own register, which already holds
> +	   it in lane 0, so bring in the other lane from operand 3.  */
> +	return "ins\t%0.<Vetype>[1], %3.<Vetype>[1]";
> +      case 2:
> +	return "ins\t%0.<Vetype>[0], %<vwcore>1";
> +      case 3:
> +	return "ld1\t{%0.<Vetype>}[0], %1";
> +      default:
> +	gcc_unreachable ();
> +      }
> +  }
> +  [(set_attr "type" "neon_ins<q>, neon_ins<q>, neon_from_gp<q>,
> +		     neon_load1_one_lane<q>")]
> +)
> +
>  (define_insn "@aarch64_simd_vec_set<mode>"
>    [(set (match_operand:VALL_F16 0 "register_operand" "=w,w,w")
>  	(vec_merge:VALL_F16
> @@ -1401,6 +1439,36 @@
>    }
>  )
>  
> +;; Lane copy between two two-element vectors.  When the source and
> +;; destination lanes are the same, the copy reads one lane from each input,
> +;; so it can equally be done by inserting the live lane of either input into
> +;; the other.  Offer both directions and let the register allocator tie
> +;; whichever input already occupies the destination, so that neither lane
> +;; needs an extra move.  Other lane combinations are left to the general
> +;; pattern below, where only the destination can be tied.
> +(define_insn "*aarch64_simd_vec_copy_lane_same<mode>"
> +  [(set (match_operand:VP_2E 0 "register_operand" "=w,w")
> +	(vec_merge:VP_2E
> +	    (vec_duplicate:VP_2E
> +	      (vec_select:<VEL>
> +		(match_operand:VP_2E 3 "register_operand" "w,0")
> +		(parallel
> +		  [(match_operand:SI 4 "immediate_operand" "i,i")])))
> +	    (match_operand:VP_2E 1 "register_operand" "0,w")
> +	    (match_operand:SI 2 "immediate_operand" "i,i")))]
> +  "TARGET_SIMD
> +   && ENDIAN_LANE_N (2, INTVAL (operands[4])) == 1
> +   && INTVAL (operands[2]) == (BYTES_BIG_ENDIAN ? 1 : 2)"
> +  {
> +    if (which_alternative == 0)
> +      return "ins\t%0.<Vetype>[1], %3.<Vetype>[1]";
> +    /* The destination is operand 3, which already holds lane 1, so bring
> +       in lane 0 from operand 1.  */
> +    return "ins\t%0.<Vetype>[0], %1.<Vetype>[0]";
> +  }
> +  [(set_attr "type" "neon_ins<q>")]
> +)

It looks like both patterns could use the "@..." syntax to specify the
alternatives, rather than switching on which_alternative.  There isn't
the ENDIAN_LANE_N stuff that makes some of the other patterns require
C++ code.

The patterns could then use the new alternative syntax, where the
constraints and attributes are on the same line as the asm.

Otherwise this LGTM, but I'm not sure that I should approve.

Thanks,
Richard

>  (define_insn "@aarch64_simd_vec_copy_lane<mode>"
>    [(set (match_operand:VALL_F16 0 "register_operand" "=w")
>  	(vec_merge:VALL_F16
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr123951_1.c b/gcc/testsuite/gcc.target/aarch64/pr123951_1.c
> new file mode 100644
> index 00000000000..3c195505a89
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr123951_1.c
> @@ -0,0 +1,41 @@
> +/* PR tree-optimization/123951.  Copying a lane between two vectors must
> +   remain a single INS (or ZIP) whichever lane pair is used, on both
> +   endiannesses.  */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#include <arm_neon.h>
> +
> +#define BUILD_TEST(TYPE, Q1, Q2, SUFFIX, INDEX1, INDEX2)		\
> +TYPE __attribute__((noinline,noclone))					\
> +test_copy##Q1##_lane##Q2##_##SUFFIX##_##INDEX1##INDEX2 (TYPE a, TYPE b) \
> +{									\
> +  return vcopy##Q1##_lane##Q2##_##SUFFIX (a, INDEX1, b, INDEX2);	\
> +}
> +
> +BUILD_TEST (uint64x2_t,  q, q, u64, 0, 0)
> +BUILD_TEST (int64x2_t,   q, q, s64, 0, 0)
> +BUILD_TEST (float64x2_t, q, q, f64, 0, 0)
> +/* { dg-final { scan-assembler-times "ins\\tv0.d\\\[0\\\], v1.d\\\[0\\\]" 3 } } */
> +BUILD_TEST (uint64x2_t,  q, q, u64, 1, 1)
> +BUILD_TEST (int64x2_t,   q, q, s64, 1, 1)
> +BUILD_TEST (float64x2_t, q, q, f64, 1, 1)
> +/* { dg-final { scan-assembler-times "ins\\tv0.d\\\[1\\\], v1.d\\\[1\\\]" 3 } } */
> +BUILD_TEST (uint64x2_t,  q, q, u64, 1, 0)
> +BUILD_TEST (int64x2_t,   q, q, s64, 1, 0)
> +BUILD_TEST (float64x2_t, q, q, f64, 1, 0)
> +/* { dg-final { scan-assembler-times "zip1\\tv0.2d, v0.2d, v1.2d" 3 } } */
> +BUILD_TEST (uint64x2_t,  q, q, u64, 0, 1)
> +BUILD_TEST (int64x2_t,   q, q, s64, 0, 1)
> +BUILD_TEST (float64x2_t, q, q, f64, 0, 1)
> +/* { dg-final { scan-assembler-times "zip2\\tv0.2d, v1.2d, v0.2d" 3 } } */
> +BUILD_TEST (uint32x2_t,  , , u32, 0, 0)
> +BUILD_TEST (int32x2_t,   , , s32, 0, 0)
> +BUILD_TEST (float32x2_t, , , f32, 0, 0)
> +/* { dg-final { scan-assembler-times "ins\\tv0.s\\\[0\\\], v1.s\\\[0\\\]" 3 } } */
> +BUILD_TEST (uint32x2_t,  , , u32, 1, 1)
> +BUILD_TEST (int32x2_t,   , , s32, 1, 1)
> +BUILD_TEST (float32x2_t, , , f32, 1, 1)
> +/* { dg-final { scan-assembler-times "ins\\tv0.s\\\[1\\\], v1.s\\\[1\\\]" 3 } } */
> +
> +/* { dg-final { scan-assembler-not "\\tmov\\t" } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr123951_2.c b/gcc/testsuite/gcc.target/aarch64/pr123951_2.c
> new file mode 100644
> index 00000000000..59770b6b000
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr123951_2.c
> @@ -0,0 +1,35 @@
> +/* PR tree-optimization/123951.  Like pr123951_1.c, but for generic vector
> +   shuffles, including ones that only become lane inserts after being
> +   re-encoded to a wider element mode.  */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +typedef unsigned long long v2di __attribute__((vector_size (16)));
> +typedef unsigned int v4si __attribute__((vector_size (16)));
> +
> +v2di
> +shuffle_03 (v2di a, v2di b)
> +{
> +  return __builtin_shuffle (a, b, (v2di) { 0, 3 });
> +}
> +
> +v2di
> +shuffle_21 (v2di a, v2di b)
> +{
> +  return __builtin_shuffle (a, b, (v2di) { 2, 1 });
> +}
> +
> +v4si
> +shuffle_0167 (v4si a, v4si b)
> +{
> +  return __builtin_shuffle (a, b, (v4si) { 0, 1, 6, 7 });
> +}
> +
> +v4si
> +shuffle_4523 (v4si a, v4si b)
> +{
> +  return __builtin_shuffle (a, b, (v4si) { 4, 5, 2, 3 });
> +}
> +
> +/* { dg-final { scan-assembler-times "\\tins\\t" 4 } } */
> +/* { dg-final { scan-assembler-not "\\tmov\\t" } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c b/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c
> index 1fb91a34c42..5acfe33a576 100644
> --- a/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c
> +++ b/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c
> @@ -43,7 +43,7 @@ void d1 (_Float16 *restrict a, _Float16 *restrict b, _Float16 *res, int n)
>  ** ...
>  ** 	fadd	v[0-9]+.2d, v[0-9]+.2d, v[0-9]+.2d
>  ** 	fsub	v[0-9]+.2d, v[0-9]+.2d, v[0-9]+.2d
> -** 	ins	v[0-9]+.d\[1\], v[0-9]+.d\[1\]
> +** 	ins	v[0-9]+.d\[0\], v[0-9]+.d\[0\]
>  ** ...
>  */
>  void e1 (double *restrict a, double *restrict b, double *res, int n)
> diff --git a/gcc/testsuite/gcc.target/aarch64/simd/addsub_2.c b/gcc/testsuite/gcc.target/aarch64/simd/addsub_2.c
> index 87424c94f24..023adb46ece 100644
> --- a/gcc/testsuite/gcc.target/aarch64/simd/addsub_2.c
> +++ b/gcc/testsuite/gcc.target/aarch64/simd/addsub_2.c
> @@ -43,7 +43,7 @@ void d1 (_Float16 *restrict a, _Float16 *restrict b, _Float16 *res, int n)
>  ** ...
>  ** 	fsub	v[0-9]+.2d, v[0-9]+.2d, v[0-9]+.2d
>  ** 	fadd	v[0-9]+.2d, v[0-9]+.2d, v[0-9]+.2d
> -** 	ins	v[0-9]+.d\[1\], v[0-9]+.d\[1\]
> +** 	ins	v[0-9]+.d\[0\], v[0-9]+.d\[0\]
>  ** ...
>  */
>  void e1 (double *restrict a, double *restrict b, double *res, int n)