Tamar Christina <[email protected]> writes: > 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.
Yeah, but that's what I think makes the above problematic. The condition should pass even after RA. It's a major blind spot that we don't have RTL checkers to verify things like that. Also, any subreg used in place of vec_select will be a lowpart subreg, so if we do still end up testing for subregs, I think it should be based on that. The condition above doesn't look correct, since any little-endian subreg would have a SUBREG_BYTE of 0. Thanks, Richard
