Re: [Qemu-devel] [PATCH v3b 08/18] target/arm: Implement SVE reverse within elements

[Peter Maydell <peter.maydell@linaro.org>]

On 30 May 2018 at 19:01, Richard Henderson <richard.henderson@linaro.org> wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper-sve.h    | 14 +++++++++++++
>  target/arm/sve_helper.c    | 41 +++++++++++++++++++++++++++++++-------
>  target/arm/translate-sve.c | 38 +++++++++++++++++++++++++++++++++++
>  target/arm/sve.decode      |  7 +++++++
>  4 files changed, 93 insertions(+), 7 deletions(-)

> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index 941a098234..f8579a25e3 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -238,6 +238,26 @@ static inline uint64_t expand_pred_s(uint8_t byte)
>      return word[byte & 0x11];
>  }
>
> +/* Swap 16-bit words within a 32-bit word.  */
> +static inline uint32_t hswap32(uint32_t h)
> +{
> +    return rol32(h, 16);
> +}
> +
> +/* Swap 16-bit words within a 64-bit word.  */
> +static inline uint64_t hswap64(uint64_t h)
> +{
> +    uint64_t m = 0x0000ffff0000ffffull;
> +    h = rol64(h, 32);
> +    return ((h & m) << 16) | ((h >> 16) & m);
> +}
> +
> +/* Swap 32-bit words within a 64-bit word.  */
> +static inline uint64_t wswap64(uint64_t h)
> +{
> +    return rol64(h, 32);
> +}
> +
>  #define LOGICAL_PPPP(NAME, FUNC) \
>  void HELPER(NAME)(void *vd, void *vn, void *vm, void *vg, uint32_t desc)  \
>  {                                                                         \
> @@ -616,6 +636,20 @@ DO_ZPZ(sve_neg_h, uint16_t, H1_2, DO_NEG)
>  DO_ZPZ(sve_neg_s, uint32_t, H1_4, DO_NEG)
>  DO_ZPZ_D(sve_neg_d, uint64_t, DO_NEG)
>
> +DO_ZPZ(sve_revb_h, uint16_t, H1_2, bswap16)
> +DO_ZPZ(sve_revb_s, uint32_t, H1_4, bswap32)
> +DO_ZPZ_D(sve_revb_d, uint64_t, bswap64)
> +
> +DO_ZPZ(sve_revh_s, uint32_t, H1_4, hswap32)
> +DO_ZPZ_D(sve_revh_d, uint64_t, hswap64)
> +
> +DO_ZPZ_D(sve_revw_d, uint64_t, wswap64)
> +
> +DO_ZPZ(sve_rbit_b, uint8_t, H1, revbit8)
> +DO_ZPZ(sve_rbit_h, uint16_t, H1_2, revbit16)
> +DO_ZPZ(sve_rbit_s, uint32_t, H1_4, revbit32)
> +DO_ZPZ_D(sve_rbit_d, uint64_t, revbit64)
> +
>  /* Three-operand expander, unpredicated, in which the third operand is "wide".
>   */
>  #define DO_ZZW(NAME, TYPE, TYPEW, H, OP)                       \
> @@ -1587,13 +1621,6 @@ void HELPER(sve_rev_b)(void *vd, void *vn, uint32_t desc)
>      }
>  }
>
> -static inline uint64_t hswap64(uint64_t h)
> -{
> -    uint64_t m = 0x0000ffff0000ffffull;
> -    h = rol64(h, 32);
> -    return ((h & m) << 16) | ((h >> 16) & m);
> -}
> -

You added this in patch 2, I think -- you could avoid the code
motion here by putting it in the right place to start with.

>  void HELPER(sve_rev_h)(void *vd, void *vn, uint32_t desc)
>  {
>      intptr_t i, j, opr_sz = simd_oprsz(desc);

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM