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]>; [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"

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.

Thanks,
Richard