Re: [ARM] PR98435: Missed optimization in expanding vector constructor

[Christophe LYON <christophe.lyon@foss.st.com>]

On 28/06/2021 10:40, Kyrylo Tkachov via Gcc-patches wrote:
>
>> -----Original Message-----
>> From: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
>> Sent: 28 June 2021 09:38
>> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>> Cc: Christophe Lyon <christophe.lyon@linaro.org>; gcc Patches <gcc-
>> patches@gcc.gnu.org>
>> Subject: Re: [ARM] PR98435: Missed optimization in expanding vector
>> constructor
>>
>> On Thu, 24 Jun 2021 at 22:01, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>> wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
>>>> Sent: 14 June 2021 09:02
>>>> To: Christophe Lyon <christophe.lyon@linaro.org>
>>>> Cc: gcc Patches <gcc-patches@gcc.gnu.org>; Kyrylo Tkachov
>>>> <Kyrylo.Tkachov@arm.com>
>>>> Subject: Re: [ARM] PR98435: Missed optimization in expanding vector
>>>> constructor
>>>>
>>>> On Wed, 9 Jun 2021 at 15:58, Prathamesh Kulkarni
>>>> <prathamesh.kulkarni@linaro.org> wrote:
>>>>> On Fri, 4 Jun 2021 at 13:15, Christophe Lyon
>> <christophe.lyon@linaro.org>
>>>> wrote:
>>>>>> On Fri, 4 Jun 2021 at 09:27, Prathamesh Kulkarni via Gcc-patches
>>>>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>>>> Hi,
>>>>>>> As mentioned in PR, for the following test-case:
>>>>>>>
>>>>>>> #include <arm_neon.h>
>>>>>>>
>>>>>>> bfloat16x4_t f1 (bfloat16_t a)
>>>>>>> {
>>>>>>>    return vdup_n_bf16 (a);
>>>>>>> }
>>>>>>>
>>>>>>> bfloat16x4_t f2 (bfloat16_t a)
>>>>>>> {
>>>>>>>    return (bfloat16x4_t) {a, a, a, a};
>>>>>>> }
>>>>>>>
>>>>>>> Compiling with arm-linux-gnueabi -O3 -mfpu=neon -mfloat-
>> abi=softfp
>>>>>>> -march=armv8.2-a+bf16+fp16 results in f2 not being vectorized:
>>>>>>>
>>>>>>> f1:
>>>>>>>          vdup.16 d16, r0
>>>>>>>          vmov    r0, r1, d16  @ v4bf
>>>>>>>          bx      lr
>>>>>>>
>>>>>>> f2:
>>>>>>>          mov     r3, r0  @ __bf16
>>>>>>>          adr     r1, .L4
>>>>>>>          ldrd    r0, [r1]
>>>>>>>          mov     r2, r3  @ __bf16
>>>>>>>          mov     ip, r3  @ __bf16
>>>>>>>          bfi     r1, r2, #0, #16
>>>>>>>          bfi     r0, ip, #0, #16
>>>>>>>          bfi     r1, r3, #16, #16
>>>>>>>          bfi     r0, r2, #16, #16
>>>>>>>          bx      lr
>>>>>>>
>>>>>>> This seems to happen because vec_init pattern in neon.md has VDQ
>>>> mode
>>>>>>> iterator, which doesn't include V4BF. In attached patch, I changed
>>>>>>> mode
>>>>>>> to VDQX which seems to work for the test-case, and the compiler
>> now
>>>> generates:
>>>>>>> f2:
>>>>>>>          vdup.16 d16, r0
>>>>>>>          vmov    r0, r1, d16  @ v4bf
>>>>>>>          bx      lr
>>>>>>>
>>>>>>> However, the pattern is also gated on TARGET_HAVE_MVE and I am
>>>> not
>>>>>>> sure if either VDQ or VDQX are correct modes for MVE since MVE
>> has
>>>>>>> only 128-bit vectors ?
>>>>>>>
>>>>>> I think patterns common to both Neon and MVE should be moved to
>>>>>> vec-common.md, I don't know why such patterns were left in
>> neon.md.
>>>>> Since we end up calling neon_expand_vector_init for both NEON and
>> MVE,
>>>>> I am not sure if we should separate the pattern ?
>>>>> Would it make sense to FAIL if the mode size isn't 16 bytes for MVE as
>>>>> in attached patch so
>>>>> it will call neon_expand_vector_init only for 128-bit vectors ?
>>>>> Altho hard-coding 16 in the pattern doesn't seem a good idea to me
>> either.
>>>> ping https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572342.html
>>>> (attaching patch as text).
>>>>
>>> --- a/gcc/config/arm/neon.md
>>> +++ b/gcc/config/arm/neon.md
>>> @@ -459,10 +459,12 @@
>>>   )
>>>
>>>   (define_expand "vec_init<mode><V_elem_l>"
>>> -  [(match_operand:VDQ 0 "s_register_operand")
>>> +  [(match_operand:VDQX 0 "s_register_operand")
>>>      (match_operand 1 "" "")]
>>>     "TARGET_NEON || TARGET_HAVE_MVE"
>>>   {
>>> +  if (TARGET_HAVE_MVE && GET_MODE_SIZE (GET_MODE
>> (operands[0])) != 16)
>>> +    FAIL;
>>>     neon_expand_vector_init (operands[0], operands[1]);
>>>     DONE;
>>>   })
>>>
>>> I think we should move this to vec-common.md like Christophe said.
>>> Perhaps rather than making it FAIL for non-16 MVE sizes we just disable it in
>> the expander condition?
>>> "TARGET_NEON || (TARGET_HAVE_MVE && GET_MODE_SIZE (<
>> VDQ>mode) != 16)"
>> Is it OK to use <MODE>mode ? Because using <VDQ>mode resulted in lot
>> of build errors.
>> Also, I think the comparison should be inverted, ie, GET_MODE_SIZE
>> (<MODE>mode) == 16 since
>> we want to make the pattern pass if target is MVE and vector size is 16 bytes ?
>> Do these changes in attached patch look OK ?
> Yes, you're right.


Can't this be ARM_HAVE_<MODE>_ARITH like in most expanders in vec-common.md?

(maybe with a && !TARGET_REALLY_IWMMXT if needed)


Christophe


> Ok.
> Thanks,
> Kyrill
>
>
>> Thanks,
>> Prathamesh
>>> Thanks,
>>> Kyrill
>>>
>>>> Thanks,
>>>> Prathamesh
>>>>> Thanks,
>>>>> Prathamesh
>>>>>> That being said, I suggest you look at other similar patterns in
>>>>>> vec-common.md, most of which are gated on
>>>>>> ARM_HAVE_<MODE>_ARITH
>>>>>> and possibly beware of issues with iwmmxt :-)
>>>>>>
>>>>>> Christophe
>>>>>>
>>>>>>> Thanks,
>>>>>>> Prathamesh