Re: [PATCH 1/1] aarch64: Avoid extra move for two-element vector lane copies [PR123951]

Richard Sandiford <[email protected]> writes:
> I suppose it would be more constructive to sketch what I'm actually
> suggesting.  I still think your original idea of having a vec_select
> pattern and a scalar pattern is the way to go.  Due to the vec_select-to-
> subreg simplificaton rule that you mentioned (and contributed), the
> vec_select will always be for the high part.  In other words, it will
> always be for architectural lane 1.  So for your suggestion:
>
> (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]))"
>   {
>
> operand 4 should in practice always be 0 for big-endian and 1 for
> little-endian.  The other case should be folded away.  We could test
> for that using:
>
>   "TARGET_SIMD
>    && exact_log2 (INTVAL (operands[2])) >= 0
>    && ENDIAN_LANE_N (2, INTVAL (operands[4])) == 1"
>
> (i.e. just changing the last line).
>
> This is then (as you say) a replacement for the existing VP_2E
> subset of the aarch64_simd_vec_copy_lane<mode> pattern.

Sigh.  But of course that drops the bit about making sure that lane 1
of the result comes from lane 1 of operand 3.  I had that in an earlier
version but somehow convinced myself that it wasn't necessary.

So we do still need the equivalent of:

   && INTVAL (operands[4]) == exact_log2 (INTVAL (operands[2]))

In this context, the full condition could be:

   "TARGET_SIMD
    && ENDIAN_LANE_N (2, INTVAL (operands[4])) == 1
    && INTVAL (operands[2]) == (BYTES_BIG_ENDIAN ? 1 : 2)"

for better contrast with the suggestion below.

Then alternative 0, which ties operands 0 and 1, would be:

   ins\t%0[1], %3[1]

for both endiannesses.  Alternative 1 would similarly be:

   ins\t%0[0], %1[0]

for both endiannesses.

Maybe there's an argument that simplify-rtx.cc should simplify this
to the vec_set form.  I.e., for little-endian:

  (vec_merge:V2X
    (vec_duplicate:V2X (vec_select:X V1 [(const_int 1)]))
    V2
    (const_int 2))

should become:

  (vec_merge:V2X
    (vec_duplicate:V2X (lowpart V2))
    V1
    (const_int 1))

and similarly for big-endian with the constants adjusted appropriately.

We would then use the vec_set form for both endiannesses.  That also has
the advantage of coping better with spilling, since vec_set has the GPR
and memory alternatives to fall back on.

Maybe that's a bit target-specific?  But it is simpler in rtl terms too.

> The scalar pattern will always be for the low part.  In other words,
> it will always be for architectural lane 0.  So for your suggestion:
>
> (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))"
>   {
>
> I think we should replace the condition with:
>
>   "TARGET_SIMD && exact_log2 (INTVAL (operands[2])) >= 0"
>
> This pattern is then a replacement for the VP_2E subset of the existing
> @aarch64_simd_vec_set<mode> pattern.  Because of that, it would be good
> to include the existing ?r and Utv alternatives too.  The thing that
> we're adding is alternative 1 above.

Similarly here, we'd need:

   "TARGET_SIMD && INTVAL (operands[2]) == (BYTES_BIG_ENDIAN ? 2 : 1)"

And the alternative that ties operands 0 and 2 would be:

   ins\t%0[1], %1[1]

for both endiannesses.  The other alternatives would insert operand 2
into %0[0].

Adding the condition means that we'd need to retain the existing vec_set
for inserting operand 2 into %0[1].

Richard