[PATCH][GCC][Arm] Add HF modes to ANY iterators

[Tamar Christina]

The 02/13/2019 10:57, Kyrill Tkachov wrote:
> Hi Tamar
> 
> On 2/13/19 10:33 AM, Tamar Christina wrote:
> > Hi All,
> >
> > The iterators ANY64 and ANY128 are used in various general split 
> > patterns and
> > are supposed to contain any 64 bit and 128 bit modes respectively.
> >
> > For some reason these patterns had HI but not HF.  This adds HF so 
> > that general
> > 64 and 128 bit splits are generated for these modes as well.  These 
> > are required
> > by various split patterns that expect them to be there.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and <native regtest 
> > still running> issues.
> >
> Please do this on an arm-none-linux-gnueabihf target.
> 
> Though I suspect this is just a placeholder from a boilerplate ;)

Oops, yes sorry, used the wrong template.. and forgot to fill in the results.. oh my.

Bootstrapped and regtested on arm-none-linux-gnueabihf and no issues :)

> 
> > Ok for trunk?
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
> >
> >         PR target/88850
> >         * config/arm/iterators.md (ANY64): Add V4HF,
> >         (ANY128): Add V8HF.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
> >
> >         PR target/88850
> >         * gcc.target/arm/pr88850-2.c: New test.
> >
> > -- 
> 
> diff --git a/gcc/config/arm/iterators.md b/gcc/config/arm/iterators.md
> index c33e572c3e89c3dc5848bd6b825d618481247558..4ac048a0c609273691c264c97ccf6cd47b43943b 100644
> --- a/gcc/config/arm/iterators.md
> +++ b/gcc/config/arm/iterators.md
> @@ -24,11 +24,11 @@
>   ;;----------------------------------------------------------------------------
>   
>   ;; A list of modes that are exactly 64 bits in size. This is used to expand
> -;; some splits that are the same for all modes when operating on ARM
> +;; some splits that are the same for all modes when operating on ARM
>   ;; registers.
> -(define_mode_iterator ANY64 [DI DF V8QI V4HI V2SI V2SF])
> +(define_mode_iterator ANY64 [DI DF V8QI V4HI V4HF V2SI V2SF])
>   
> -(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V4SI V4SF])
> +(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V8HF V4SI V4SF])
>   
> 
> I see ANY128 is used in the move_hi_quad_<mode> and move_lo_quad_<mode> expanders in neon.md.
> Does the use of HF modes need guarding by TARGET_NEON_FP16INST there?

I don't think it does, the patterns would generate a V_HALF move which should match either
mov<mode> with the VH iterator or *neon_mov<mode> with VQXMOV, both of which already
contain V4HF. This is because I think it's just going to move them around in s registers without caring what's
in them as the bits are not being manipulated so you don't need fp16 instructions.

> Would be good to see a test exercising this change...

Would you be able to guide me in the direction of how? It seems practically impossible to generate this pattern from C.
The move_hi_quad and move_lo_quad are inserts, but using neon intrinsics like vcombine_f16 doesn't generate this RTL pattern.
The only thing that uses these things is vec_pack_trunc_  and the only thing that seems to use this is the auto vectorizer.

The autovectorizer won't ever generate a V8HF case because my previous patch to enable autovectorization of vector HFmodes was
deferred to GCC 10 as the patch series was cannibalized.

I'm more than happy not to change ANY128 if you prefer.

Thanks,
Tamar

> 
> 
>   ;; A list of integer modes that are up to one word long
>   (define_mode_iterator QHSI [QI HI SI])
> diff --git a/gcc/testsuite/gcc.target/arm/pr88850-2.c b/gcc/testsuite/gcc.target/arm/pr88850-2.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..fee2a0fe3a227ea4652bea2aa0e7335d20a7e9b5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr88850-2.c
> @@ -0,0 +1,19 @@
> +/* PR target/88850.  */
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O2 -march=armv7-a -mfloat-abi=softfp -fdump-rtl-final" } */
> +/* { dg-add-options arm_neon_fp16 } */
> +/* { dg-require-effective-target arm_soft_ok } */
> +/* { dg-require-effective-target arm_neon_fp16_ok } */
> +
> 
> arm_soft_ok checks if it's okay to add -mfloat-abi=soft, but you're adding softfp.
> Is this right?
> 
> Thanks,
> Kyrill
> 
> +#include <arm_neon.h>
> +
> +extern void c (int, float16x4_t);
> +
> +void a (float16x4_t b)
> +{
> +  c (0, b);
> +}
> +
> +
> +/* Check that these 64-bit moves are done in SI.  */
> +/* { dg-final { scan-rtl-dump "_movsi_vfp" "final" } } *
> 

--