Re: [PATCH]AArch64 relax constraints on FP16 insn PR108172

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

Tamar Christina <Tamar.Christina@arm.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandiford@arm.com>
>> Sent: Wednesday, December 21, 2022 10:31 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]AArch64 relax constraints on FP16 insn PR108172
>> 
>> Tamar Christina <tamar.christina@arm.com> writes:
>> > Hi All,
>> >
>> > The move, load, load, store, dup, basically all the non arithmetic
>> > FP16 instructions use baseline armv8-a HF support, and so do not
>> > require the Armv8.2-a extensions.  This relaxes the instructions.
>> >
>> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>> >
>> > Ok for master?
>> >
>> > Thanks,
>> > Tamar
>> >
>> > gcc/ChangeLog:
>> >
>> > 	PR target/108172
>> > 	* config/aarch64/aarch64-simd.md (*aarch64_simd_movv2hf): Relax
>> > 	TARGET_SIMD_F16INST to TARGET_SIMD.
>> > 	* config/aarch64/aarch64.cc (aarch64_classify_vector_mode): Re-
>> order.
>> > 	* config/aarch64/iterators.md (VMOVE): Drop
>> TARGET_SIMD_F16INST.
>> >
>> > gcc/testsuite/ChangeLog:
>> >
>> > 	PR target/108172
>> > 	* gcc.target/aarch64/pr108172.c: New test.
>> 
>> OK, thanks.
>> 
>> I think we need better tests for this area though.  The VMOVE uses include
>> aarch64_dup_lane<mode>, which uses <Vtype>, which has no definition for
>> V2HF, so we get:
>> 
>>     return "dup\t%0.<Vtype>, %1.h[%2]";
>> 
>> The original patch defined Vtype to "2h", but using that here would have
>> generated an invalid instruction.
>> 
>> We also have unexpanded <Vtype>s in the pairwise operations:
>> 
>>     "faddp\t%h0, %1.<Vtype>",
>>     "fmaxp\t%h0, %1.<Vtype>",
>>     "fminp\t%h0, %1.<Vtype>",
>>     "fmaxnmp\t%h0, %1.<Vtype>",
>>     "fminnmp\t%h0, %1.<Vtype>",
>> 
>
> Right, the pairwise bit should have been reverted, the tests were all In that patch.
>
>> Would it be easy (using a combination of this patch and a follow-on patch) to
>> wind the V2HF support back to a state that makes sense on its own, without
>> the postponed pairwise support?  Or would it be simpler to revert
>> 2cba118e538ba0b7582af7f9fb5ba2dfbb772f8e for GCC 13 and revisit this in
>> GCC 14, alongside the original motivating use case?
>
> The pairwise and the dup should be undone, as evidence that they don't trigger
> currently. The mov make sense on their own already and improve various codegen
> around shorts on their own.

OK, sounds good, thanks.

> I won't be revisiting pairwise operations again, so I'll remove the remnants from this
> Patch.

Ack.  Sorry for causing frustration in the review for that series.

Richard

>> Richard
>> 
>> > --- inline copy of patch --
>> > diff --git a/gcc/config/aarch64/aarch64-simd.md
>> > b/gcc/config/aarch64/aarch64-simd.md
>> > index
>> >
>> a62b6deaf4a57e570074d7d894e6fac13779f6fb..8a9f655d547285ec7bdc173086
>> 30
>> > 8d7d44a8d482 100644
>> > --- a/gcc/config/aarch64/aarch64-simd.md
>> > +++ b/gcc/config/aarch64/aarch64-simd.md
>> > @@ -164,7 +164,7 @@ (define_insn "*aarch64_simd_movv2hf"
>> >  		"=w, m,  m,  w, ?r, ?w, ?r, w, w")
>> >  	(match_operand:V2HF 1 "general_operand"
>> >  		"m,  Dz, w,  w,  w,  r,  r, Dz, Dn"))]
>> > -  "TARGET_SIMD_F16INST
>> > +  "TARGET_SIMD
>> >     && (register_operand (operands[0], V2HFmode)
>> >         || aarch64_simd_reg_or_zero (operands[1], V2HFmode))"
>> >     "@
>> > diff --git a/gcc/config/aarch64/aarch64.cc
>> > b/gcc/config/aarch64/aarch64.cc index
>> >
>> 73515c174fa4fe7830527e7eabd91c4648130ff4..d1c0476321b79bc6aded350d24
>> ea
>> > 5d556c796519 100644
>> > --- a/gcc/config/aarch64/aarch64.cc
>> > +++ b/gcc/config/aarch64/aarch64.cc
>> > @@ -3616,6 +3616,8 @@ aarch64_classify_vector_mode (machine_mode
>> mode)
>> >      case E_V4x2DFmode:
>> >        return TARGET_FLOAT ? VEC_ADVSIMD | VEC_STRUCT : 0;
>> >
>> > +    /* 32-bit Advanced SIMD vectors.  */
>> > +    case E_V2HFmode:
>> >      /* 64-bit Advanced SIMD vectors.  */
>> >      case E_V8QImode:
>> >      case E_V4HImode:
>> > @@ -3634,7 +3636,6 @@ aarch64_classify_vector_mode (machine_mode
>> mode)
>> >      case E_V8BFmode:
>> >      case E_V4SFmode:
>> >      case E_V2DFmode:
>> > -    case E_V2HFmode:
>> >        return TARGET_FLOAT ? VEC_ADVSIMD : 0;
>> >
>> >      default:
>> > diff --git a/gcc/config/aarch64/iterators.md
>> > b/gcc/config/aarch64/iterators.md index
>> >
>> a521dbde1ec42c0c442a9ca3dd52c9727d116399..70742520984d30158e62a38c9
>> 2ab
>> > ea82b2dac059 100644
>> > --- a/gcc/config/aarch64/iterators.md
>> > +++ b/gcc/config/aarch64/iterators.md
>> > @@ -204,8 +204,7 @@ (define_mode_iterator VALL_F16 [V8QI V16QI V4HI
>> > V8HI V2SI V4SI V2DI  ;; All Advanced SIMD modes suitable for moving,
>> > loading, and storing  ;; including V2HF  (define_mode_iterator VMOVE
>> > [V8QI V16QI V4HI V8HI V2SI V4SI V2DI
>> > -			     V4HF V8HF V4BF V8BF V2SF V4SF V2DF
>> > -			     (V2HF "TARGET_SIMD_F16INST")])
>> > +			     V2HF V4HF V8HF V4BF V8BF V2SF V4SF V2DF])
>> >
>> >
>> >  ;; The VALL_F16 modes except the 128-bit 2-element ones.
>> > diff --git a/gcc/testsuite/gcc.target/aarch64/pr108172.c
>> > b/gcc/testsuite/gcc.target/aarch64/pr108172.c
>> > new file mode 100644
>> > index
>> >
>> 0000000000000000000000000000000000000000..b29054fdb1d6e602755bc9308
>> 9f1
>> > edec4eb53b8e
>> > --- /dev/null
>> > +++ b/gcc/testsuite/gcc.target/aarch64/pr108172.c
>> > @@ -0,0 +1,30 @@
>> > +/* { dg-do compile } */
>> > +/* { dg-options "-O0" } */
>> > +
>> > +typedef _Float16 v4hf __attribute__ ((vector_size (8))); typedef
>> > +_Float16 v2hf __attribute__ ((vector_size (4)));
>> > +
>> > +v4hf
>> > +v4hf_abi_1 (v4hf a)
>> > +{
>> > +  return a;
>> > +}
>> > +
>> > +v4hf
>> > +v4hf_abi_3 (v4hf a, v4hf b, v4hf c)
>> > +{
>> > +  return c;
>> > +}
>> > +
>> > +v4hf
>> > +v4hf_abi_4 (v4hf a, v4hf b, v4hf c, v4hf d) {
>> > +  return d;
>> > +}
>> > +
>> > +v2hf
>> > +v2hf_test (v2hf a, v2hf b, v2hf c, v2hf d) {
>> > +  return b;
>> > +}
>> > +