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

Richard Sandiford <[email protected]> writes:
> Tamar Christina <[email protected]> writes:
>>> > 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].
>
> Guess I'm just stating common ground in the first few paragraphs below, but:
>
> Subregs like that are only allowed in one special case: a 64-bit
> highpart of a 128-bit AdvSIMD register.  The fact that that case is
> allowed is due to a defective definition of REGMODE_NATURAL_SIZE, which
> Robin is in the process of fixing.
>
> Once the 64-bit highpart of a 128-bit register case is used, we have
> already lost, since if we have:
>
>   (subreg:DI (reg:V2DI R) 0) [big-endian]
>   (subreg:DI (reg:V2DI R) 8) [little-endian]
>
> LRA will not allocate R to a SIMD register.  (This is partly due to the
> simplifiable_subregs/valid_mode_changes_for_regno stuff.)
>
> So the subregs that I listed above wouldn't become references to architectural
> lane 1.  Instead, the RAs would spill R and reload the high 64 bits into the
> lowpart of a SIMD register, which would become operand 2.  I think this
> would normally happen through memory, but perhaps GPRs are also possible.
>
> So after RA, operand 2 would always be the lowpart of a SIMD register
> (lane 0), and would have the right contents, whatever the SUBREG_BYTE
> was before RA.  The behaviour would be correct, but the spilling would
> make the subregs above very inefficient.
>
> Perhaps you were wanting to prevent the inefficient 64-bit highpart
> subregs from being formed.  But I'm not sure the condition would do that.
> Even if it does, the same thing would happen for any other DImode SIMD
> register_operand that had one of the forms above, including the existing
> vec_merge-of-vec_duplicate patterns.  The inefficiency comes from the
> spilling that LRA is bound to emit, and that inefficiency would apply
> wherever the subreg occurs, not just to this pattern.  The pattern
> doesn't seem like a special case.
>
> So I still think we can ignore whether operand 2 is a subreg.  It's the
> RA's job to ensure that operand 2 occupies the lowpart of a register.

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.

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.

Thanks,
Richard