Re: [PATCH 5/8]AArch64 aarch64: Make existing V2HF be usable.

[Richard Sandiford <richard.sandiford@arm.com>]

Tamar Christina <Tamar.Christina@arm.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandiford@arm.com>
>> Sent: Tuesday, December 6, 2022 10:28 AM
>> To: Tamar Christina <Tamar.Christina@arm.com>
>> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw
>> <Richard.Earnshaw@arm.com>; Marcus Shawcroft
>> <Marcus.Shawcroft@arm.com>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>> Subject: Re: [PATCH 5/8]AArch64 aarch64: Make existing V2HF be usable.
>> 
>> Tamar Christina <Tamar.Christina@arm.com> writes:
>> > Hi,
>> >
>> >
>> >> This name might cause confusion with the SVE iterators, where FULL
>> >> means "every bit of the register is used".  How about something like
>> >> VMOVE instead?
>> >>
>> >> With this change, I guess VALL_F16 represents "The set of all modes
>> >> for which the vld1 intrinsics are provided" and VMOVE or whatever is
>> >> "All Advanced SIMD modes suitable for moving, loading, and storing".
>> >> That is, VMOVE extends VALL_F16 with modes that are not manifested
>> >> via intrinsics.
>> >>
>> >
>> > Done.
>> >
>> >> Where is the 2h used, and is it valid syntax in that context?
>> >>
>> >> Same for later instances of 2h.
>> >
>> > They are, but they weren't meant to be in this patch.  They belong in
>> > a separate FP16 series that I won't get to finish for GCC 13 due not
>> > being able to finish writing all the tests.  I have moved them to that patch
>> series though.
>> >
>> > While the addp patch series has been killed, this patch is still good
>> > standalone and improves codegen as shown in the updated testcase.
>> >
>> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>> >
>> > Ok for master?
>> >
>> > Thanks,
>> > Tamar
>> >
>> > gcc/ChangeLog:
>> >
>> > 	* config/aarch64/aarch64-simd.md (*aarch64_simd_movv2hf): New.
>> > 	(mov<mode>, movmisalign<mode>, aarch64_dup_lane<mode>,
>> > 	aarch64_store_lane0<mode>, aarch64_simd_vec_set<mode>,
>> > 	@aarch64_simd_vec_copy_lane<mode>, vec_set<mode>,
>> > 	reduc_<optab>_scal_<mode>, reduc_<fmaxmin>_scal_<mode>,
>> > 	aarch64_reduc_<optab>_internal<mode>,
>> aarch64_get_lane<mode>,
>> > 	vec_init<mode><Vel>, vec_extract<mode><Vel>): Support V2HF.
>> > 	(aarch64_simd_dupv2hf): New.
>> > 	* config/aarch64/aarch64.cc (aarch64_classify_vector_mode):
>> > 	Add E_V2HFmode.
>> > 	* config/aarch64/iterators.md (VHSDF_P): New.
>> > 	(V2F, VMOVE, nunits, Vtype, Vmtype, Vetype, stype, VEL,
>> > 	Vel, q, vp): Add V2HF.
>> > 	* config/arm/types.md (neon_fp_reduc_add_h): New.
>> >
>> > gcc/testsuite/ChangeLog:
>> >
>> > 	* gcc.target/aarch64/sve/slp_1.c: Update testcase.
>> >
>> > --- inline copy of patch ---
>> >
>> > diff --git a/gcc/config/aarch64/aarch64-simd.md
>> > b/gcc/config/aarch64/aarch64-simd.md
>> > index
>> >
>> f4152160084d6b6f34bd69f0ba6386c1ab50f77e..487a31010245accec28e779661
>> e6
>> > c2d578fca4b7 100644
>> > --- a/gcc/config/aarch64/aarch64-simd.md
>> > +++ b/gcc/config/aarch64/aarch64-simd.md
>> > @@ -19,10 +19,10 @@
>> >  ;; <http://www.gnu.org/licenses/>.
>> >
>> >  (define_expand "mov<mode>"
>> > -  [(set (match_operand:VALL_F16 0 "nonimmediate_operand")
>> > -	(match_operand:VALL_F16 1 "general_operand"))]
>> > +  [(set (match_operand:VMOVE 0 "nonimmediate_operand")
>> > +	(match_operand:VMOVE 1 "general_operand"))]
>> >    "TARGET_SIMD"
>> > -  "
>> > +{
>> >    /* Force the operand into a register if it is not an
>> >       immediate whose use can be replaced with xzr.
>> >       If the mode is 16 bytes wide, then we will be doing @@ -46,12
>> > +46,11 @@ (define_expand "mov<mode>"
>> >        aarch64_expand_vector_init (operands[0], operands[1]);
>> >        DONE;
>> >      }
>> > -  "
>> > -)
>> > +})
>> >
>> >  (define_expand "movmisalign<mode>"
>> > -  [(set (match_operand:VALL_F16 0 "nonimmediate_operand")
>> > -        (match_operand:VALL_F16 1 "general_operand"))]
>> > +  [(set (match_operand:VMOVE 0 "nonimmediate_operand")
>> > +        (match_operand:VMOVE 1 "general_operand"))]
>> >    "TARGET_SIMD && !STRICT_ALIGNMENT"
>> >  {
>> >    /* This pattern is not permitted to fail during expansion: if both
>> > arguments @@ -73,6 +72,16 @@ (define_insn
>> "aarch64_simd_dup<mode>"
>> >    [(set_attr "type" "neon_dup<q>, neon_from_gp<q>")]
>> >  )
>> >
>> > +(define_insn "aarch64_simd_dupv2hf"
>> > +  [(set (match_operand:V2HF 0 "register_operand" "=w")
>> > +	(vec_duplicate:V2HF
>> > +	  (match_operand:HF 1 "register_operand" "0")))]
>> 
>> Seems like this should be "w" rather than "0", since SLI is a two-register
>> instruction.
>
> Yes, but for a dup it's only valid when the same register is used. i.e. it has to
> write into the original src register.

Ah, right.  In that case it might be better to use %d0 for the source
operand:

  For operands to match in a particular case usually means that they
  are identical-looking RTL expressions.  But in a few special cases
  specific kinds of dissimilarity are allowed.  For example, @code{*x}
  as an input operand will match @code{*x++} as an output operand.
  For proper results in such cases, the output template should always
  use the output-operand's number when printing the operand.

Thanks,
Richard