[PATCH][AArch64] Use intrinsics for upper saturating shift right

[Christophe Lyon]

Hi,


On Thu, 5 Nov 2020 at 17:12, David Candler via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi Richard,
>
> Thanks for the feedback.
>
> Richard Sandiford <richard.sandiford@arm.com> writes:
> > > diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
> > > index 4f33dd936c7..f93f4e29c89 100644
> > > --- a/gcc/config/aarch64/aarch64-builtins.c
> > > +++ b/gcc/config/aarch64/aarch64-builtins.c
> > > @@ -254,6 +254,10 @@ aarch64_types_binop_imm_qualifiers[SIMD_MAX_BUILTIN_ARGS]
> > >  #define TYPES_GETREG (aarch64_types_binop_imm_qualifiers)
> > >  #define TYPES_SHIFTIMM (aarch64_types_binop_imm_qualifiers)
> > >  static enum aarch64_type_qualifiers
> > > +aarch64_types_ternop_s_imm_qualifiers[SIMD_MAX_BUILTIN_ARGS]
> > > +  = { qualifier_none, qualifier_none, qualifier_none, qualifier_immediate};
> > > +#define TYPES_SHIFT2IMM (aarch64_types_ternop_s_imm_qualifiers)
> > > +static enum aarch64_type_qualifiers
> > >  aarch64_types_shift_to_unsigned_qualifiers[SIMD_MAX_BUILTIN_ARGS]
> > >    = { qualifier_unsigned, qualifier_none, qualifier_immediate };
> > >  #define TYPES_SHIFTIMM_USS (aarch64_types_shift_to_unsigned_qualifiers)
> > > @@ -265,14 +269,16 @@ static enum aarch64_type_qualifiers
> > >  aarch64_types_unsigned_shift_qualifiers[SIMD_MAX_BUILTIN_ARGS]
> > >    = { qualifier_unsigned, qualifier_unsigned, qualifier_immediate };
> > >  #define TYPES_USHIFTIMM (aarch64_types_unsigned_shift_qualifiers)
> > > +#define TYPES_USHIFT2IMM (aarch64_types_ternopu_imm_qualifiers)
> > > +static enum aarch64_type_qualifiers
> > > +aarch64_types_shift2_to_unsigned_qualifiers[SIMD_MAX_BUILTIN_ARGS]
> > > +  = { qualifier_unsigned, qualifier_unsigned, qualifier_none, qualifier_immediate };
> > > +#define TYPES_SHIFT2IMM_UUSS (aarch64_types_shift2_to_unsigned_qualifiers)
> > >
> > >  static enum aarch64_type_qualifiers
> > >  aarch64_types_ternop_s_imm_p_qualifiers[SIMD_MAX_BUILTIN_ARGS]
> > >    = { qualifier_none, qualifier_none, qualifier_poly, qualifier_immediate};
> > >  #define TYPES_SETREGP (aarch64_types_ternop_s_imm_p_qualifiers)
> > > -static enum aarch64_type_qualifiers
> > > -aarch64_types_ternop_s_imm_qualifiers[SIMD_MAX_BUILTIN_ARGS]
> > > -  = { qualifier_none, qualifier_none, qualifier_none, qualifier_immediate};
> > >  #define TYPES_SETREG (aarch64_types_ternop_s_imm_qualifiers)
> > >  #define TYPES_SHIFTINSERT (aarch64_types_ternop_s_imm_qualifiers)
> > >  #define TYPES_SHIFTACC (aarch64_types_ternop_s_imm_qualifiers)
> >
> > Very minor, but I think it would be better to keep
> > aarch64_types_ternop_s_imm_qualifiers where it is and define
> > TYPES_SHIFT2IMM here rather than above.  For better or worse,
> > the current style seems to be to keep the defines next to the
> > associated arrays, rather than group them based on the TYPES_* name.
> >
> > > diff --git a/gcc/config/aarch64/aarch64-simd-builtins.def b/gcc/config/aarch64/aarch64-simd-builtins.def
> > > index d1b21102b2f..0b82b9c072b 100644
> > > --- a/gcc/config/aarch64/aarch64-simd-builtins.def
> > > +++ b/gcc/config/aarch64/aarch64-simd-builtins.def
> > > @@ -285,6 +285,13 @@
> > >    BUILTIN_VSQN_HSDI (USHIFTIMM, uqshrn_n, 0, ALL)
> > >    BUILTIN_VSQN_HSDI (SHIFTIMM, sqrshrn_n, 0, ALL)
> > >    BUILTIN_VSQN_HSDI (USHIFTIMM, uqrshrn_n, 0, ALL)
> > > +  /* Implemented by aarch64_<sur>q<r>shr<u>n2_n<mode>.  */
> > > +  BUILTIN_VQN (SHIFT2IMM_UUSS, sqshrun2_n, 0, ALL)
> > > +  BUILTIN_VQN (SHIFT2IMM_UUSS, sqrshrun2_n, 0, ALL)
> > > +  BUILTIN_VQN (SHIFT2IMM, sqshrn2_n, 0, ALL)
> > > +  BUILTIN_VQN (USHIFT2IMM, uqshrn2_n, 0, ALL)
> > > +  BUILTIN_VQN (SHIFT2IMM, sqrshrn2_n, 0, ALL)
> > > +  BUILTIN_VQN (USHIFT2IMM, uqrshrn2_n, 0, ALL)
> >
> > Using ALL is a holdover from the time (until a few weeks ago) when we
> > didn't record function attributes.  New intrinsics should therefore
> > have something more specific than ALL.
> >
> > We discussed offline whether the Q flag side effect of the intrinsics
> > should be observable or not, and the conclusion was that it shouldn't.
> > I think we can therefore treat these functions as pure functions,
> > meaning that they should have flags NONE rather than ALL.
> >
> > For that reason, I think we should also remove the Set_Neon_Cumulative_Sat
> > and CHECK_CUMULATIVE_SAT parts of the test (sorry).
> >
> > Other than that, the patch looks good to go.
> >
> > Thanks,
> > Richard
>
> I've updated the patch with TYPES_SHIFT2IMM moved, the builtins changed
> to NONE, and the Q flag portion of the tests removed.
>

It looks like you forgot that these tests are shared with the arm target, and
since there intrinsics are not supported on that target you should make sure
they are skipped (there are several examples in advsimd-intrinsics/)

Christophe

> Thanks,
> David