Hi Rohith, Sorry for the delay.
> -----Original Message----- > From: Rohith Kapelli <[email protected]> > Sent: 19 July 2026 11:44 > To: [email protected] > Cc: Richard Sandiford <[email protected]>; Kyrylo Tkachov > <[email protected]>; Richard Earnshaw <[email protected]>; > Rohith Kapelli <[email protected]> > Subject: [PATCH 1/1] aarch64: Avoid extra move for two-element vector lane > copies [PR123951] > > Since r14-3381 the vec_extract/vec_set idiom used by the vcopy*_lane* > intrinsics is folded into a VEC_PERM_EXPR by forwprop, so the constant > permute expander rather than combine now decides what to emit for it. > > For two-element vectors, an insert permutation matches two different > decompositions at the same time: { 0, 3 } is op0 with element 1 of op1 > inserted at index 1, but equally op1 with element 0 of op0 inserted at > index 0. aarch64_evpc_ins always picks the first form, tying the > result to op0. The match.pd canonicalization of VEC_PERM_EXPR swaps > the operands whenever the selector starts with an element of the > second vector, so for half of the lane combinations the vector that > the intrinsic inserts into arrives as op1, and the register allocator > then has to satisfy the tie with an extra move. > > On big endian the lane numbering flip in arm_neon.h makes > vcopyq_laneq_u64 (a, 1, b, 1) take exactly that path, so what used to > be a single INS in GCC 13 became INS plus MOV and vect_copy_lane_1.c > started failing. Little endian has the same problem for > vcopyq_laneq_u64 (a, 0, b, 0), and __builtin_shuffle permutations of > this shape have always been pessimized this way, even before the > intrinsics started using this path. > > The expander cannot make the right choice: each input contributes > exactly one lane, the canonical VEC_PERM_EXPR is the same for both > readings, and which form is cheaper only becomes known during register > allocation. Emit a plain two-input vec_merge instead and give it an > insn with both tying alternatives, so that the allocator resolves the > tie and no move is needed for either lane combination, on either > endianness. > I don't think you actually need the expander changes as the majority of the heavy lifting here is being done by the new VP_2E pattern you added. So I think what you want is to change aarch64_simd_vec_copy_lane and to exclude VP_2E modes and then make a new pattern to cover these having the same operand swap as your new pattern. So something like (define_insn "@aarch64_simd_vec_copy_lane<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 && exact_log2 (INTVAL (operands[2])) >= 0 && INTVAL (operands[4]) == exact_log2 (INTVAL (operands[2]))" { > The addsub_1.c and addsub_2.c body checks match the same > addend/subtrahend merge; with the tie left to the allocator the freely > allocated case now inserts into lane 0 rather than lane 1, still a > single INS, so their expected output is updated accordingly. > > gcc/ChangeLog: > > PR tree-optimization/123951 > * config/aarch64/aarch64-simd.md > (@aarch64_simd_vec_merge<mode>): > New insn. > * config/aarch64/aarch64.cc (aarch64_evpc_ins): Look for both > single-insert decompositions of the permutation and emit the new > insn when both match. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/123951 > * gcc.target/aarch64/pr123951_1.c: New test. > * gcc.target/aarch64/pr123951_2.c: New test. > * gcc.target/aarch64/simd/addsub_1.c: Update the 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 | 31 ++++++++ > gcc/config/aarch64/aarch64.cc | 74 ++++++++++++++----- > 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 +- > 6 files changed, 166 insertions(+), 19 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..a46629763c3 100644 > --- a/gcc/config/aarch64/aarch64-simd.md > +++ b/gcc/config/aarch64/aarch64-simd.md > @@ -1422,6 +1422,37 @@ > [(set_attr "type" "neon_ins<q>")] > ) > > +;; A two-element vec_merge takes one lane from each input, so it can be > +;; carried out by inserting the live lane of either input into the other. > +;; Offer both forms and leave the register allocator to pick the input to > +;; tie to the destination, so that no extra move is needed. > +(define_insn "@aarch64_simd_vec_merge<mode>" > + [(set (match_operand:VP_2E 0 "register_operand" "=w,w") > + (vec_merge:VP_2E > + (match_operand:VP_2E 1 "register_operand" "w,0") > + (match_operand:VP_2E 2 "register_operand" "0,w") > + (match_operand:SI 3 "immediate_operand" "i,i")))] > + "TARGET_SIMD > + && (INTVAL (operands[3]) == 1 || INTVAL (operands[3]) == 2)" > + { > + int elt = INTVAL (operands[3]) == 1 ? 0 : 1; > + if (which_alternative == 0) > + { > + /* The destination already holds the value of operand 2; insert > + the live lane of operand 1. */ > + operands[3] = GEN_INT (ENDIAN_LANE_N (<nunits>, elt)); > + return "ins\t%0.<Vetype>[%3], %1.<Vetype>[%3]"; > + } > + else > + { > + /* Conversely, insert the live lane of operand 2. */ > + operands[3] = GEN_INT (ENDIAN_LANE_N (<nunits>, 1 - elt)); > + return "ins\t%0.<Vetype>[%3], %2.<Vetype>[%3]"; > + } > + } > + [(set_attr "type" "neon_ins<q>")] > +) This pattern then becomes nearly identical to the one I suggested in the cover letter but with (define_insn "*aarch64_simd_vec_copy_lane_same<mode>_subreg" [(set (match_operand:VP_2E 0 "register_operand" "=w,w") (vec_merge:VP_2E (vec_duplicate:VP_2E (match_operand:<VEL> 2 "register_operand" "w,0")) (match_operand:VP_2E 1 "register_operand" "0,w") (match_operand:SI 3 "immediate_operand" "i,i")))] "TARGET_SIMD && INTVAL (operands[3]) == 2 && SUBREG_P (operands[2]) && known_eq (SUBREG_BYTE (operands[2]), GET_MODE_SIZE (<VEL>mode))" { This catches the subreg case where the vec_select is folded to a subreg by Simplify RTX. Before reload it'll be a subreg but after reload the subreg gets removed and replaced by the hardreg. For the print pattern you can then use operands[2] = gen_rtx_REG (<MODE>mode, reg_or_subregno (src)); after doing the conditional register selection (i.e. if alternative 1 we read from operands[1] rather than operands[2].) Since it's a subreg you also don't need lane corrections here. Those two patterns should fix the optimization without needing the expansion changes. Thanks, Tamar > + > (define_insn > "@aarch64_simd_vec_copy_lane_<vswap_width_name><mode>" > [(set (match_operand:VALL_F16_NO_V2Q 0 "register_operand" "=w") > (vec_merge:VALL_F16_NO_V2Q > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index a1f91dd425e..c1dbc794c29 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -28480,10 +28480,14 @@ aarch64_evpc_ins (struct > expand_vec_perm_d *d) > /* to_constant is safe since this routine is specific to Advanced SIMD > vectors. */ > nelt = d->perm.length ().to_constant (); > - rtx insv = d->op0; > > - HOST_WIDE_INT idx = -1; > + /* The permutation is a match if it is an identity on one of the inputs > + with a single element taken from the other input. Look for both > + decompositions, since for two-element vectors they can hold at the > + same time: { 0, 3 } is op0 with element 1 of op1 inserted at index 1, > + but equally op1 with element 0 of op0 inserted at index 0. */ > > + HOST_WIDE_INT idx0 = -1; > for (unsigned HOST_WIDE_INT i = 0; i < nelt; i++) > { > HOST_WIDE_INT elt; > @@ -28491,35 +28495,71 @@ aarch64_evpc_ins (struct > expand_vec_perm_d *d) > return false; > if (elt == (HOST_WIDE_INT) i) > continue; > - if (idx != -1) > + if (idx0 != -1) > { > - idx = -1; > + idx0 = -1; > break; > } > - idx = i; > + idx0 = i; > } > > - if (idx == -1) > + HOST_WIDE_INT idx1 = -1; > + for (unsigned HOST_WIDE_INT i = 0; i < nelt; i++) > { > - insv = d->op1; > - for (unsigned HOST_WIDE_INT i = 0; i < nelt; i++) > + if (d->perm[i].to_constant () == (HOST_WIDE_INT) (i + nelt)) > + continue; > + if (idx1 != -1) > { > - if (d->perm[i].to_constant () == (HOST_WIDE_INT) (i + nelt)) > - continue; > - if (idx != -1) > - return false; > - idx = i; > + idx1 = -1; > + break; > } > + idx1 = i; > + } > > - if (idx == -1) > - return false; > + if (idx0 != -1 && idx1 != -1) > + { > + /* Both decompositions match, so each input contributes exactly one > + element: the result can be computed by inserting the live lane of > + either input into the other. Which one is cheaper depends on the > + register allocation, since the insn ties its destination to the > + vector being inserted into; a fixed choice here would make the > + allocator emit an extra move whenever the other input was the > + better candidate (PR123951). Emit a plain two-input vec_merge > + and leave the choice of lane to the allocator. */ > + gcc_assert (nelt == 2); > + if (d->testing_p) > + return true; > + > + insn_code icode = code_for_aarch64_simd_vec_merge (mode); > + expand_operand ops[4]; > + create_output_operand (&ops[0], d->target, mode); > + create_input_operand (&ops[1], d->op0, mode); > + create_input_operand (&ops[2], d->op1, mode); > + /* Mask bit I selects element I of op0; idx1 is the one element > + that does not come from op1. */ > + create_integer_operand (&ops[3], HOST_WIDE_INT_1U << idx1); > + expand_insn (icode, 4, ops); > + return true; > } > > + rtx insv; > + HOST_WIDE_INT idx; > + if (idx0 != -1) > + { > + insv = d->op0; > + idx = idx0; > + } > + else if (idx1 != -1) > + { > + insv = d->op1; > + idx = idx1; > + } > + else > + return false; > + > if (d->testing_p) > return true; > > - gcc_assert (idx != -1); > - > unsigned extractindex = d->perm[idx].to_constant (); > rtx extractv = d->op0; > if (extractindex >= nelt) > 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) > -- > 2.53.0
