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

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

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

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.

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-2.diff --]
[-- Type: application/octet-stream, Size: 541 bytes --]

diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
index 6a6573317cf..27dd672ca76 100644
--- 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;
 })