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)
