> -----Original Message----- > From: Richard Sandiford <[email protected]> > Sent: 31 July 2026 14:41 > To: Tamar Christina <[email protected]> > Cc: Rohith Kapelli <[email protected]>; [email protected]; > Kyrylo Tkachov <[email protected]>; Richard Earnshaw > <[email protected]> > Subject: Re: [PATCH 1/1] aarch64: Avoid extra move for two-element vector > lane copies [PR123951] > > 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.
Yes, I'm aware of that. I'm also aware that we don't actually recheck the condition after reload. Once the pattern is selected it stays valid. So since the RTL shape didn't change it still matches. This is also why I mentioned because the byte offset isn't lane offset it already account for the correct location, so the lanes don't have to be endian swapped. So yes. I am aware of this. Thanks, Tamar > > Thanks, > Richard
