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 <richard.hender...@linaro.org>
Sent: Friday, April 24, 2020 2:37 PM
To: Stephen Long <stepl...@quicinc.com>; qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org; Ana Pazos <apa...@quicinc.com>
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 <stepl...@quicinc.com>
>
> 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~
