RE: [PATCH RFC] target/arm: Implement SVE2 TBL, TBX

[Stephen Long]

Oh, maybe I misread the manual description for SVE2 TBL, but I thought Zm was 
the indexes register and the loop compares the index from Zm with the total 
number of elems, table_elems.

-----Original Message-----
From: Richard Henderson <address@hidden>
Sent: Friday, April 24, 2020 2:37 PM
To: Stephen Long <address@hidden>; address@hidden
Cc: address@hidden; Ana Pazos <address@hidden>
Subject: [EXT] Re: [PATCH RFC] target/arm: Implement SVE2 TBL, TBX

On 4/23/20 9:42 AM, Stephen Long wrote:
> Signed-off-by: Stephen Long <address@hidden>
>
> These insns don't show up under any SVE2 categories in the manual. But
> if you lookup each insn, you'll find they have SVE2 variants.
> ---
>  target/arm/helper-sve.h    | 10 +++++++
>  target/arm/sve.decode      |  5 ++++
>  target/arm/sve_helper.c    | 53 ++++++++++++++++++++++++++++++++++++++
>  target/arm/translate-sve.c | 20 ++++++++++++++
>  4 files changed, 88 insertions(+)
>
> diff --git a/target/arm/helper-sve.h b/target/arm/helper-sve.h index
> f6ae814021..54d20575e8 100644
> --- a/target/arm/helper-sve.h
> +++ b/target/arm/helper-sve.h
> @@ -2687,3 +2687,13 @@ DEF_HELPER_FLAGS_5(sve2_sqrdcmlah_zzzz_s, 
> TCG_CALL_NO_RWG,
>                     void, ptr, ptr, ptr, ptr, i32)
> DEF_HELPER_FLAGS_5(sve2_sqrdcmlah_zzzz_d, TCG_CALL_NO_RWG,
>                     void, ptr, ptr, ptr, ptr, i32)
> +
> +DEF_HELPER_FLAGS_5(sve2_tbl_b, TCG_CALL_NO_RWG, void, ptr, ptr, ptr,
> +ptr, i32) DEF_HELPER_FLAGS_5(sve2_tbl_h, TCG_CALL_NO_RWG, void, ptr,
> +ptr, ptr, ptr, i32) DEF_HELPER_FLAGS_5(sve2_tbl_s, TCG_CALL_NO_RWG,
> +void, ptr, ptr, ptr, ptr, i32) DEF_HELPER_FLAGS_5(sve2_tbl_d,
> +TCG_CALL_NO_RWG, void, ptr, ptr, ptr, ptr, i32)
> +
> +DEF_HELPER_FLAGS_4(sve2_tbx_b, TCG_CALL_NO_RWG, void, ptr, ptr, ptr,
> +i32) DEF_HELPER_FLAGS_4(sve2_tbx_h, TCG_CALL_NO_RWG, void, ptr, ptr,
> +ptr, i32) DEF_HELPER_FLAGS_4(sve2_tbx_s, TCG_CALL_NO_RWG, void, ptr,
> +ptr, ptr, i32) DEF_HELPER_FLAGS_4(sve2_tbx_d, TCG_CALL_NO_RWG, void,
> +ptr, ptr, ptr, i32)
> diff --git a/target/arm/sve.decode b/target/arm/sve.decode index
> 3a2a4a7f1c..483fbf0dcc 100644
> --- a/target/arm/sve.decode
> +++ b/target/arm/sve.decode
> @@ -1387,3 +1387,8 @@ UMLSLT_zzzw     01000100 .. 0 ..... 010 111 ..... ..... 
>  @rda_rn_rm
>
>  CMLA_zzzz       01000100 esz:2 0 rm:5 0010 rot:2 rn:5 rd:5  ra=%reg_movprfx
>  SQRDCMLAH_zzzz  01000100 esz:2 0 rm:5 0011 rot:2 rn:5 rd:5
> ra=%reg_movprfx
> +
> +### SVE2 Table Lookup (three sources)
> +
> +TBL_zzz         00000101 .. 1 ..... 00101 0 ..... .....  @rd_rn_rm
> +TBX_zzz         00000101 .. 1 ..... 00101 1 ..... .....  @rd_rn_rm
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c index
> 55e2c32f03..d1e91da02a 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -2968,6 +2968,59 @@ DO_TBL(sve_tbl_d, uint64_t, )
>
>  #undef TBL
>
> +#define DO_SVE2_TBL(NAME, TYPE, H) \
> +void HELPER(NAME)(void *vd, void *vn1, void *vm, void *vn2, uint32_t desc)  \
> +{                                                                           \
> +    intptr_t i, opr_sz = simd_oprsz(desc);                                  \
> +    uintptr_t elem = opr_sz / sizeof(TYPE);                                 \
> +    TYPE *d = vd, *n1 = vn1, *n2 = vn2, *m = vm;                            \
> +    ARMVectorReg tmp1, tmp2;                                                \

Only one temp needed.

> +    if (unlikely(vd == vn1)) {                                              \
> +        n1 = memcpy(&tmp1, vn1, opr_sz);                                    \
> +    }                                                                       \
> +    if (unlikely(vd == vn2)) {                                              \
> +        n2 = memcpy(&tmp2, vn2, opr_sz);                                    \
> +    }

Better with else if here.
Because vd cannot overlap both vn1 or vn2, only one of them.
                                             \
> +    for (i = 0; i < elem; i++) {                                            \
> +        TYPE j = m[H(i)];                                                   \
> +        d[H(i)] = j < (elem * 2) ? n1[H(j)] : 0;                            \
> +                                                                            \
> +        TYPE k = m[H(elem + i)];                                            \
> +        d[H(elem + i)] = k < (elem * 2) ? n2[H(k)] : 0;                     \
> +    }

First, the indexing is wrong.

Note that you're range checking vs elem * 2, but only indexing a single vector.
 Thus you must be indexing into the next vector.

This should look more like

    TYPE j = m[H(i)];
    TYPE r = 0;

    if (j < elem) {
        r = n1[H(j)];
    } else if (j < 2 * elem) {
        r = n2[H(j - elem)];
    }
    d[H(i)] = r;

Second, this is one case where I'd prefer to share code with AArch64.  It would 
be worthwhile to rearrange both sve1 and advsimd to use a common set of helpers.

> +static bool trans_TBL_zzz(DisasContext *s, arg_rrr_esz *a)

_zzz is not helpful here.  The SVE1 insn also operates on 3 registers, and thus 
could logically be _zzz too.

Better might be _double, after double_table = TRUE, or maybe just _2 just in 
case SVE3 adds a variant with more table registers.


r~