> -----Original Message----- > From: Richard Sandiford <[email protected]> > Sent: 31 July 2026 16:45 > 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: > >> -----Original Message----- > >> From: Tamar Christina > >> Sent: 31 July 2026 14:51 > >> To: Richard Sandiford <[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] > >> > >> > -----Original Message----- > >> > From: Tamar Christina > >> > Sent: 31 July 2026 14:47 > >> > To: 'Richard Sandiford' <[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] > >> > > >> > > -----Original Message----- > >> > > From: Richard Sandiford <[email protected]> > >> > > Sent: 31 July 2026 14:41 > >> > > To: Tamar Christina <[email protected]> > >> > > Cc: Rohith Kapelli <[email protected]>; gcc- > [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. > >> > >> And to follow up, that concern is easily addressed with > >> > >> || (reload_completed && REG_P (operands[2])) added to the condition. > >> > >> Then the condition and the RTL match. So this is still perfectly fine. > > > > Since Richard had an objection, he's an alternate approach. These operations > > are essentially a vec_concat rather than a vec_merge. The issues can be > avoided > > if we actually treat them as such. > > > > For LE the first pattern stays the same. We also add these > > > > (define_insn "*aarch64_vec_concat_lane<mode>" > > [(set (match_operand:VP_2E 0 "register_operand" "=w,w") > > (vec_concat:VP_2E > > (vec_select:<VEL> > > (match_operand:VP_2E 1 "register_operand" "0,w") > > (parallel > > [(match_operand:SI 3 "immediate_operand" "i,i")])) > > (vec_select:<VEL> > > (match_operand:VP_2E 2 "register_operand" "w,0") > > (parallel > > [(match_operand:SI 4 "immediate_operand" "i,i")]))))] > > "TARGET_SIMD > > && INTVAL (operands[3]) == 0 > > && INTVAL (operands[4]) == 1" > > { > > ... > > > > Which essentially is saying concat two vectors together from a vec_select. > > This then becomes your INS. > > > > Then for BE you rewrite the vec_merge into the vec_concat using e.g. > > > > (define_insn_and_split > "*aarch64_simd_vec_copy_lane_same<mode>_be_lowpart" > > [(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 > > && BYTES_BIG_ENDIAN > > && !reload_completed > > && INTVAL (operands[3]) == 2 > > && SUBREG_P (operands[2]) > > && subreg_lowpart_p (operands[2])" > > "#" > > "&& true" > > ... > > > > This changes the codegen to target > > > > (vec_concat:V2DF > > (vec_select:DF (reg:V2DF x) [(const_int 0)]) > > (vec_select:DF (reg:V2DF y) [(const_int 1)])) > > > > Which represents the INS without needing the subreg lanes and the subreg > > goes away way before reload. > > > > Hopefully Richard is happy with this one. > > But I'm not sure what this subreg stuff is trying to achieve. Are you > trying to force the case where operand 2 is a "natural scalar" (handwavy > term) through a different pattern? If so, which one? > > The existing vec_merge-of-vec_duplicate patterns don't seem to care > where the scalar comes from. If the operand is a plain pseudo REG > defined by a GPR operation then LRA will generate a GPR-to-FPR move. > One of those is going to be needed somewhere in that case. > > So I was more wondering why your original suggestion needed the: > > && SUBREG_P (operands[2]) > && known_eq (SUBREG_BYTE (operands[2]), GET_MODE_SIZE > (<VEL>mode))" > > and couldn't just be: > > "TARGET_SIMD && INTVAL (operands[3]) == 2" >
Because this pattern is *only* valid for upper inserts, lower inserts is handled by the first one, the aarch64_simd_vec_copy_lane<mode>. Operand two, since the vec_merge pattern is a bit mask, 2 means your destination is lane 1, the known_eq (SUBREG_BYTE (operands[2]), GET_MODE_SIZE> (<VEL>mode)) comparison is only valid if the subreg is the high part of a 2 lane vector. Or in other words, lane 1. So this pattern is only handling ins[1], ins[1]. And in effect was only targeting big-endian. In the vec_concat case I made it explicit with BYTES_BIG_ENDIAN. It's dealing with the fact that the vec_select of the lowpart of a 2 lane vector Is folded into subregs by simplify-rtx. So it has to deal with it as a subreg. > Very minor, but if operand 3 really has to be 2, the pattern could > just hard-code (const_int 2). > > It looks on the face of it though like 1 or 2 would be ok for your > original pattern, with the choice of asm instruction depending on > that choice and the endianness. I might have misunderstood though. Possibly, but the 1 constraint , i.e. insert in lane 0, from lane 0, is already handled, elsewhere. Inserting into lane 0 from lane 1 is a zip2, and already handled correctly. And so are the inverses. The only fixes here are for 1 into 1. Tamar. > > Thanks, > Richard
