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

[Richard Henderson]

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 <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~
>