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

[Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 11625 bytes --]

On Tue, 3 Aug 2021 at 14:59, Christophe Lyon
<christophe.lyon.oss@gmail.com> wrote:
>
>
>
> On Tue, Jul 6, 2021 at 11:26 AM Prathamesh Kulkarni via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>
>> On Tue, 6 Jul 2021 at 13:33, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> wrote:
>> >
>> >
>> >
>> > > -----Original Message-----
>> > > From: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
>> > > Sent: 06 July 2021 08:06
>> > > To: Christophe LYON <christophe.lyon@foss.st.com>
>> > > Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>; gcc Patches <gcc-
>> > > patches@gcc.gnu.org>
>> > > Subject: Re: [ARM] PR98435: Missed optimization in expanding vector
>> > > constructor
>> > >
>> > > On Thu, 1 Jul 2021 at 16:26, Prathamesh Kulkarni
>> > > <prathamesh.kulkarni@linaro.org> wrote:
>> > > >
>> > > > On Wed, 30 Jun 2021 at 20:51, Christophe LYON
>> > > > <christophe.lyon@foss.st.com> wrote:
>> > > > >
>> > > > >
>> > > > > On 29/06/2021 12:46, Prathamesh Kulkarni wrote:
>> > > > > > On Mon, 28 Jun 2021 at 14:48, Christophe LYON
>> > > > > > <christophe.lyon@foss.st.com> wrote:
>> > > > > >>
>> > > > > >> 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)
>> > > > > > I wonder if this should be ARM_HAVE_<MODE>_LDST instead since
>> > > we're
>> > > > > > initializing the vector ?
>> > > > >
>> > > > >
>> > > > > Well, it really depends on which modes you want to enable.
>> > > > >
>> > > > >
>> > > > > Looks like your move VDQ -> VDQ adds V4BF, V8BF and DI.
>> > > > >
>> > > > > Are they all OK for Neon?
>> > > > >
>> > > > > They are not OK for MVE.
>> > > > >
>> > > > > Ideally you could add testcases to cover to the supported and
>> > > > > unsupported modes for both Neon and MVE.\
>> > > > >
>> > > > > Before your patch, the expander is enabled for MVE for 64 bit modes
>> > > > > (V8QI, V4HI, V2SI): what happens in this case? Does the compiler crash
>> > > > > or is there something else preventing the match?
>> > > > Hi,
>> > > > Apparently there is VALID_MVE_MODE macro, so is it better to use:
>> > > > TARGET_NEON || (TARGET_HAVE_MVE &&
>> > > VALID_MVE_MODE(<MODE>mode))
>> > > > as in the attached patch ?
>> >
>> > The change is ok. I would like to see some testcases like Christophe suggested, but this patch just moves the expander around rather than introducing new functionality.
>> Hi Kyrill,
>> As mentioned in the first email, the patch improves code-gen for
>> following test-case:
>>
>> bfloat16x4_t f (bfloat16_t a)
>> {
>>   return (bfloat16x4_t) {a, a, a, a};
>> }
>>
>> Before patch:
>> f:
>>         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
>>
>> After patch:
>> f:
>>         vdup.16 d16, r0
>>         vmov    r0, r1, d16  @ v4bf
>>         bx      lr
>>
>> because the patch changes mode from VDQ to VDQX to accommodate bf modes.
>> I have included the test in the attached patch.
>> I think Christophe's concerns were mainly about the right modes
>> getting enabled for MVE.
>> Unfortunately, I am not sure how to test for that because the FE
>> catches invalid modes, and we don't
>> end up hitting the pattern.
>>
>
> Hi Prathamesh,
>
> The new testcase fails on arm-linux-gnueabihf:
>  FAIL: gcc.target/arm/simd/pr98435.c (test for excess errors)
> Excess errors:
> /aci-gcc-fsf/builds/gcc-fsf-gccsrc/sysroot-arm-none-linux-gnueabihf/usr/include/gnu/stubs.h:7:11: fatal error: gnu/stubs-soft.h: No such file or directory
> compilation terminated.
>
> Because you don't check whether  -mfloat-abi=softfp is actually supported.
>
> Can you fix that?
Oops, sorry about that.
The attached patch fixes the test by requiring arm_softfloat and makes
it UNSUPPORTED on arm-linux-gnueabihf.
Does it look OK ?

Thanks,
Prathamesh
>
> Thanks
>
> Christophe
>
>
>>
>> Thanks,
>> Prathamesh
>> > Thanks,
>> > Kyrill
>> >
>> > > ping https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574206.html
>> > >
>> > > Thanks,
>> > > Prathamesh
>> > > >
>> > > > Thanks,
>> > > > Prathamesh
>> > > > >
>> > > > >
>> > > > > Thanks,
>> > > > >
>> > > > >
>> > > > > Christophe
>> > > > >
>> > > > >
>> > > > > > Thanks,
>> > > > > > Prathamesh
>> > > > > >>
>> > > > > >> 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

[-- Attachment #2: pr98435-test-fix.diff --]
[-- Type: application/octet-stream, Size: 566 bytes --]

diff --git a/gcc/testsuite/gcc.target/arm/simd/pr98435.c b/gcc/testsuite/gcc.target/arm/simd/pr98435.c
index 0af8633fd56..4f6f6208bdf 100644
--- a/gcc/testsuite/gcc.target/arm/simd/pr98435.c
+++ b/gcc/testsuite/gcc.target/arm/simd/pr98435.c
@@ -1,6 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -ffast-math" } */
 /* { dg-require-effective-target arm_v8_2a_bf16_neon_ok } */
+/* { dg-require-effective-target arm_softfloat } */
 /* { dg-add-options arm_v8_2a_bf16_neon } */
 /* { dg-additional-options "-mfloat-abi=softfp -march=armv8.2-a+bf16+fp16" } */