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

[Kyrill Tkachov]

Hi Tamar,

On 2/13/19 4:31 PM, Tamar Christina wrote:
> 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.

Yeah, that's what I was getting at eventually :)

Let's leave ANY128 alone for now.

Kyrill

> 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" } } *
>>