Re: [PATCH v6 26/82] target/arm: Implement SVE2 SHRN, RSHRN

[Peter Maydell]

On Fri, 30 Apr 2021 at 21:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> v2: Fix typo in gen_shrnb_vec (laurent desnogues)
> v3: Replace DO_RSHR with an inline function
> ---
>  target/arm/helper-sve.h    |  16 ++++
>  target/arm/sve.decode      |   8 ++
>  target/arm/sve_helper.c    |  54 ++++++++++++-
>  target/arm/translate-sve.c | 160 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 236 insertions(+), 2 deletions(-)

> -#undef DO_SHR
> -#undef DO_SHL

Did we want to move the #undef DO_SHR/DO_SHL rather than just deleting them ?
(I have to admit I'm not sure to what extent undeffing all of these macros is
worth the effort -- I ran into similar minor awkwardness in the MVE
helper .c file.)

>  #undef DO_ASRD
>  #undef DO_ZPZI
>  #undef DO_ZPZI_D
>
> +#define DO_SHRNB(NAME, TYPEW, TYPEN, OP) \
> +void HELPER(NAME)(void *vd, void *vn, uint32_t desc)         \
> +{                                                            \
> +    intptr_t i, opr_sz = simd_oprsz(desc);                   \
> +    int shift = simd_data(desc);                             \
> +    for (i = 0; i < opr_sz; i += sizeof(TYPEW)) {            \
> +        TYPEW nn = *(TYPEW *)(vn + i);                       \
> +        *(TYPEW *)(vd + i) = (TYPEN)OP(nn, shift);           \
> +    }                                                        \
> +}

Doesn't this need some H() macros, the way the T version does ?

> +#define DO_SHRNT(NAME, TYPEW, TYPEN, HW, HN, OP)                  \
> +void HELPER(NAME)(void *vd, void *vn, uint32_t desc)              \
> +{                                                                 \
> +    intptr_t i, opr_sz = simd_oprsz(desc);                        \
> +    int shift = simd_data(desc);                                  \
> +    for (i = 0; i < opr_sz; i += sizeof(TYPEW)) {                 \
> +        TYPEW nn = *(TYPEW *)(vn + HW(i));                        \
> +        *(TYPEN *)(vd + HN(i + sizeof(TYPEN))) = OP(nn, shift);   \
> +    }                                                             \
> +}
> +
> +DO_SHRNB(sve2_shrnb_h, uint16_t, uint8_t, DO_SHR)
> +DO_SHRNB(sve2_shrnb_s, uint32_t, uint16_t, DO_SHR)
> +DO_SHRNB(sve2_shrnb_d, uint64_t, uint32_t, DO_SHR)
> +
> +DO_SHRNT(sve2_shrnt_h, uint16_t, uint8_t, H1_2, H1, DO_SHR)
> +DO_SHRNT(sve2_shrnt_s, uint32_t, uint16_t, H1_4, H1_2, DO_SHR)
> +DO_SHRNT(sve2_shrnt_d, uint64_t, uint32_t,     , H1_4, DO_SHR)
> +
> +DO_SHRNB(sve2_rshrnb_h, uint16_t, uint8_t, do_urshr)
> +DO_SHRNB(sve2_rshrnb_s, uint32_t, uint16_t, do_urshr)
> +DO_SHRNB(sve2_rshrnb_d, uint64_t, uint32_t, do_urshr)
> +
> +DO_SHRNT(sve2_rshrnt_h, uint16_t, uint8_t, H1_2, H1, do_urshr)
> +DO_SHRNT(sve2_rshrnt_s, uint32_t, uint16_t, H1_4, H1_2, do_urshr)
> +DO_SHRNT(sve2_rshrnt_d, uint64_t, uint32_t,     , H1_4, do_urshr)
> +
> +#undef DO_SHRNB
> +#undef DO_SHRNT

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

thanks
-- PMM