On 4/24/20 3:47 PM, Stephen Long wrote:
> 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.
That's right. You take the index from Zm just fine, but fail to apply that
index properly across Zn and Zn+1.
r~
>
> -----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~
>
