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

[Richard Sandiford]

Thanks for the patch, looks good.

David Candler <David.Candler@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