On Thu, 24 Jun 2021 at 22:01, Kyrylo Tkachov <kyrylo.tkac...@arm.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Prathamesh Kulkarni <prathamesh.kulka...@linaro.org>
> > Sent: 14 June 2021 09:02
> > To: Christophe Lyon <christophe.l...@linaro.org>
> > Cc: gcc Patches <gcc-patches@gcc.gnu.org>; Kyrylo Tkachov
> > <kyrylo.tkac...@arm.com>
> > Subject: Re: [ARM] PR98435: Missed optimization in expanding vector
> > constructor
> >
> > On Wed, 9 Jun 2021 at 15:58, Prathamesh Kulkarni
> > <prathamesh.kulka...@linaro.org> wrote:
> > >
> > > On Fri, 4 Jun 2021 at 13:15, Christophe Lyon <christophe.l...@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 ?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
pr98435-3.diff
Description: Binary data
