Re: [PATCH 13/55] target/arm: Implement MVE VCLZ

[Richard Henderson <richard.henderson@linaro.org>]

On 6/7/21 9:57 AM, Peter Maydell wrote:
> Implement the MVE VCLZ insn (and the necessary machinery
> for MVE 1-input vector ops).
> 
> Note that for non-load instructions predication is always performed
> at a byte level granularity regardless of element size (R_ZLSJ),
> and so the masking logic here differs from that used in the VLDR
> and VSTR helpers.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper-mve.h    |  4 ++++
>   target/arm/mve.decode      |  8 +++++++
>   target/arm/mve_helper.c    | 48 ++++++++++++++++++++++++++++++++++++++
>   target/arm/translate-mve.c | 43 ++++++++++++++++++++++++++++++++++
>   4 files changed, 103 insertions(+)
> 
> diff --git a/target/arm/helper-mve.h b/target/arm/helper-mve.h
> index e47d4164ae7..c5c1315b161 100644
> --- a/target/arm/helper-mve.h
> +++ b/target/arm/helper-mve.h
> @@ -32,3 +32,7 @@ DEF_HELPER_FLAGS_3(mve_vldrh_uw, TCG_CALL_NO_WG, void, env, ptr, i32)
>   DEF_HELPER_FLAGS_3(mve_vstrb_h, TCG_CALL_NO_WG, void, env, ptr, i32)
>   DEF_HELPER_FLAGS_3(mve_vstrb_w, TCG_CALL_NO_WG, void, env, ptr, i32)
>   DEF_HELPER_FLAGS_3(mve_vstrh_w, TCG_CALL_NO_WG, void, env, ptr, i32)
> +
> +DEF_HELPER_FLAGS_3(mve_vclzb, TCG_CALL_NO_WG, void, env, ptr, ptr)
> +DEF_HELPER_FLAGS_3(mve_vclzh, TCG_CALL_NO_WG, void, env, ptr, ptr)
> +DEF_HELPER_FLAGS_3(mve_vclzw, TCG_CALL_NO_WG, void, env, ptr, ptr)
> diff --git a/target/arm/mve.decode b/target/arm/mve.decode
> index 3bc5f034531..24999bf703e 100644
> --- a/target/arm/mve.decode
> +++ b/target/arm/mve.decode
> @@ -20,13 +20,17 @@
>   #
>   
>   %qd 22:1 13:3
> +%qm 5:1 1:3
>   
>   &vldr_vstr rn qd imm p a w size l u
> +&1op qd qm size
>   
>   @vldr_vstr ....... . . . . l:1 rn:4 ... ...... imm:7 &vldr_vstr qd=%qd u=0
>   # Note that both Rn and Qd are 3 bits only (no D bit)
>   @vldst_wn ... u:1 ... . . . . l:1 . rn:3 qd:3 . ... .. imm:7 &vldr_vstr
>   
> +@1op .... .... .... size:2 .. .... .... .... .... &1op qd=%qd qm=%qm
> +
>   # Vector loads and stores
>   
>   # Widening loads and narrowing stores:
> @@ -61,3 +65,7 @@ VLDR_VSTR        1110110 1 a:1 . w:1 . .... ... 111101 .......   @vldr_vstr \
>                    size=1 p=1
>   VLDR_VSTR        1110110 1 a:1 . w:1 . .... ... 111110 .......   @vldr_vstr \
>                    size=2 p=1
> +
> +# Vector miscellaneous
> +
> +VCLZ             1111 1111 1 . 11 .. 00 ... 0 0100 11 . 0 ... 0 @1op
> diff --git a/target/arm/mve_helper.c b/target/arm/mve_helper.c
> index 6a2fc1c37cd..b7c44f57c09 100644
> --- a/target/arm/mve_helper.c
> +++ b/target/arm/mve_helper.c
> @@ -196,3 +196,51 @@ DO_VSTR(vstrh_w, 4, stw, int32_t, H4)
>   
>   #undef DO_VLDR
>   #undef DO_VSTR
> +
> +/*
> + * Take the bottom bits of mask (which is 1 bit per lane) and
> + * convert to a mask which has 1s in each byte which is predicated.
> + */
> +static uint8_t mask_to_bytemask1(uint16_t mask)
> +{
> +    return (mask & 1) ? 0xff : 0;
> +}
> +
> +static uint16_t mask_to_bytemask2(uint16_t mask)
> +{
> +    static const uint16_t masks[] = { 0x0000, 0x00ff, 0xff00, 0xffff };
> +    return masks[mask & 3];
> +}
> +
> +static uint32_t mask_to_bytemask4(uint16_t mask)
> +{
> +    static const uint32_t masks[] = {
> +        0x00000000, 0x000000ff, 0x0000ff00, 0x0000ffff,
> +        0x00ff0000, 0x00ff00ff, 0x00ffff00, 0x00ffffff,
> +        0xff000000, 0xff0000ff, 0xff00ff00, 0xff00ffff,
> +        0xffff0000, 0xffff00ff, 0xffffff00, 0xffffffff,
> +    };

I'll note that

(1) the values for the mask_to_bytemask2 array overlap the first 4 values of 
the mask_to_bytemask4 array, and

(2) both of these overlap with the larger

static inline uint64_t expand_pred_b(uint8_t byte)

from SVE.  It'd be nice to share the storage, whatever the actual functional 
interface into the array.

> +#define DO_1OP(OP, ESIZE, TYPE, H, FN)                                  \
> +    void HELPER(mve_##OP)(CPUARMState *env, void *vd, void *vm)         \
> +    {                                                                   \
> +        TYPE *d = vd, *m = vm;                                          \
> +        uint16_t mask = mve_element_mask(env);                          \
> +        unsigned e;                                                     \
> +        for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) {              \
> +            TYPE r = FN(m[H(e)]);                                       \
> +            uint64_t bytemask = mask_to_bytemask##ESIZE(mask);          \

Why uint64_t and not TYPE?  Or uint32_t?

> +    if (!mve_eci_check(s)) {
> +        return true;
> +    }
> +
> +    if (!vfp_access_check(s)) {
> +        return true;
> +    }

Not the first instance, but is it worth saving 4 lines per and combining these 
into one IF?

> +#define DO_1OP(INSN, FN)                                        \
> +    static bool trans_##INSN(DisasContext *s, arg_1op *a)       \
> +    {                                                           \
> +        MVEGenOneOpFn *fns[] = {                                \

static const.


r~