[PATCH] arm: Auto-vectorization for MVE: vaddv

[Christophe Lyon]

On Wed, 26 May 2021 at 11:32, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Christophe Lyon <christophe.lyon@linaro.org> writes:
> > This patch adds support for the reduc_plus_scal optab with MVE, which
> > maps to the vaddv instruction.
> >
> > It moves the reduc_plus_scal_<mode> expander from neon.md to
> > vec-common.md and adds support for MVE to it.
> >
> > Since vaddv uses a 32-bits accumulator, we have to truncate it's
> > result.
> >
> > For instance:
> > int32_t test__s8x16 (int8_t *a) {
> >   int i;
> >   int8_t result = 0;
> >   for (i=0; i<16; i++) {
> >     result += a[i];
> >   }
> >   return result;
> > }
> > is compiled into:
> >   vldrb.8 q3, [r0]
> >   vaddv.s8        r0, q3
> >   sxtb    r0, r0
> >   bx      lr
> >
> > If we used uint8_t instead of int8_t, we still use vaddv.s8        r0, q3,
> > but truncate with uxtb    r0, r0.
> >
> > 2021-05-25  Christophe Lyon  <christophe.lyon@linaro.org>
> >
> >       gcc/
> >       * config/arm/mve.md (mve_vaddvq_<supf><mode>): Prefix with '@'.
> >       * config/arm/neon.md (reduc_plus_scal_<mode>): Move to ..
> >       * config/arm/vec-common.md: .. here. Add support for MVE.
> >
> >       gcc/testsuite/
> >       * gcc.target/arm/simd/mve-vaddv-1.c: New test.
> > ---
> >  gcc/config/arm/mve.md                         |  2 +-
> >  gcc/config/arm/neon.md                        | 13 ----------
> >  gcc/config/arm/vec-common.md                  | 26 +++++++++++++++++++
> >  .../gcc.target/arm/simd/mve-vaddv-1.c         | 26 +++++++++++++++++++
> >  4 files changed, 53 insertions(+), 14 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.target/arm/simd/mve-vaddv-1.c
> >
> > diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
> > index 133ebe93cf3..0a6ba80c99d 100644
> > --- a/gcc/config/arm/mve.md
> > +++ b/gcc/config/arm/mve.md
> > @@ -464,7 +464,7 @@ (define_insn "mve_vclsq_s<mode>"
> >  ;;
> >  ;; [vaddvq_s, vaddvq_u])
> >  ;;
> > -(define_insn "mve_vaddvq_<supf><mode>"
> > +(define_insn "@mve_vaddvq_<supf><mode>"
> >    [
> >     (set (match_operand:SI 0 "s_register_operand" "=Te")
> >       (unspec:SI [(match_operand:MVE_2 1 "s_register_operand" "w")]
> > diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
> > index 977adef5490..6a6573317cf 100644
> > --- a/gcc/config/arm/neon.md
> > +++ b/gcc/config/arm/neon.md
> > @@ -1161,19 +1161,6 @@ (define_expand "reduc_plus_scal_<mode>"
> >    DONE;
> >  })
> >
> > -(define_expand "reduc_plus_scal_<mode>"
> > -  [(match_operand:<V_elem> 0 "nonimmediate_operand")
> > -   (match_operand:VQ 1 "s_register_operand")]
> > -  "ARM_HAVE_NEON_<MODE>_ARITH && !BYTES_BIG_ENDIAN"
> > -{
> > -  rtx step1 = gen_reg_rtx (<V_HALF>mode);
> > -
> > -  emit_insn (gen_quad_halves_plus<mode> (step1, operands[1]));
> > -  emit_insn (gen_reduc_plus_scal_<V_half> (operands[0], step1));
> > -
> > -  DONE;
> > -})
> > -
> >  (define_expand "reduc_plus_scal_v2di"
> >    [(match_operand:DI 0 "nonimmediate_operand")
> >     (match_operand:V2DI 1 "s_register_operand")]
> > diff --git a/gcc/config/arm/vec-common.md b/gcc/config/arm/vec-common.md
> > index e8b2901b006..cc136e2865f 100644
> > --- a/gcc/config/arm/vec-common.md
> > +++ b/gcc/config/arm/vec-common.md
> > @@ -539,3 +539,29 @@ (define_expand "vec_store_lanesxi<mode>"
> >      emit_insn (gen_mve_vst4q<mode> (operands[0], operands[1]));
> >    DONE;
> >  })
> > +
> > +(define_expand "reduc_plus_scal_<mode>"
> > +  [(match_operand:<V_elem> 0 "nonimmediate_operand")
> > +   (match_operand:VQ 1 "s_register_operand")]
> > +  "ARM_HAVE_NEON_<MODE>_ARITH || (TARGET_HAVE_MVE && <MODE>mode != V4SFmode)
> > +   && !TARGET_REALLY_IWMMXT
> > +   && !BYTES_BIG_ENDIAN"
>
> This might be better as:
>
>   "ARM_HAVE_<MODE>_ARITH
>    && !(TARGET_HAVE_MVE && FLOAT_MODE_P (<MODE>mode))
>    && !BYTES_BIG_ENDIAN"
>
> just in case the iterator gets expanded for Neon in future.
> The TARGET_REALLY_IWMMXT test shouldn't be needed.

Indeed. I got used to adding it after several bug reports related to
my previous MVE patches when using iwmmxt...

>
> > +{
> > +  if (TARGET_NEON)
> > +    {
> > +      rtx step1 = gen_reg_rtx (<V_HALF>mode);
> > +
> > +      emit_insn (gen_quad_halves_plus<mode> (step1, operands[1]));
> > +      emit_insn (gen_reduc_plus_scal_<V_half> (operands[0], step1));
> > +    }
> > +  else
> > +    {
> > +      /* vaddv generates a 32 bits accumulator.  */
> > +      rtx op0 = gen_reg_rtx (SImode);
> > +
> > +      emit_insn (gen_mve_vaddvq (VADDVQ_S, <MODE>mode, op0, operands[1]));
> > +      emit_insn (gen_rtx_SET (operands[0], gen_rtx_SUBREG (<V_elem>mode, op0, 0)));
>
> This last line should be:
>
>       emit_move_insn (operands[0], gen_lowpart (<V_elem>mode, op0));
>
> OK with those changes, thanks.

Thanks, pushed.

>
> Richard