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
