Re: [Qemu-ppc] [PATCH v1 1/9] target-ppc: implement lxvl instruction

[Nikunj A Dadhania]

David Gibson <address@hidden> writes:

> [ Unknown signature status ]
> On Wed, Dec 07, 2016 at 11:54:54PM +0530, Nikunj A Dadhania wrote:
>> lxvl: Load VSX Vector with Length
>> 
>> Little/Big-endian Storage:
>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
>> |“T”|“h”|“i”|“s”|“ ”|“i”|“s”|“ ”|“a”|“ ”|“T”|“E”|“S”|“T”|FF|FF|
>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
>> 
>> Loading 14 bytes results in:
>> 
>> Vector (8-bit elements) in BE:
>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
>> |“T”|“h”|“i”|“s”|“ ”|“i”|“s”|“ ”|“a”|“ ”|“T”|“E”|“S”|“T”|00|00|
>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
>> 
>> Vector (8-bit elements) in LE:
>> +--+--+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
>> |00|00|“T”|“S”|“E”|“T”|“ ”|“a”|“ ”|“s”|“i”|“ ”|“s”|“i”|"h"|"T"|
>> +--+--+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
>
> Took a while to wrap my head around the semantics, but I believe this
> is correct.  However, there are a couple of nits:
>
>> 
>> Signed-off-by: Nikunj A Dadhania <address@hidden>
>> ---
>>  target-ppc/helper.h                 |  1 +
>>  target-ppc/mem_helper.c             | 33 +++++++++++++++++++++++++++++++++
>>  target-ppc/translate/vsx-impl.inc.c | 27 +++++++++++++++++++++++++++
>>  target-ppc/translate/vsx-ops.inc.c  |  1 +
>>  4 files changed, 62 insertions(+)
>> 
>> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
>> index bc39efb..d9ccafd 100644
>> --- a/target-ppc/helper.h
>> +++ b/target-ppc/helper.h
>> @@ -317,6 +317,7 @@ DEF_HELPER_3(lvewx, void, env, avr, tl)
>>  DEF_HELPER_3(stvebx, void, env, avr, tl)
>>  DEF_HELPER_3(stvehx, void, env, avr, tl)
>>  DEF_HELPER_3(stvewx, void, env, avr, tl)
>> +DEF_HELPER_4(lxvl, void, env, tl, tl, tl)
>>  DEF_HELPER_4(vsumsws, void, env, avr, avr, avr)
>>  DEF_HELPER_4(vsum2sws, void, env, avr, avr, avr)
>>  DEF_HELPER_4(vsum4sbs, void, env, avr, avr, avr)
>> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
>> index 1ab8a6e..54447a7 100644
>> --- a/target-ppc/mem_helper.c
>> +++ b/target-ppc/mem_helper.c
>> @@ -24,6 +24,7 @@
>>  
>>  #include "helper_regs.h"
>>  #include "exec/cpu_ldst.h"
>> +#include "internal.h"
>>  
>>  //#define DEBUG_OP
>>  
>> @@ -284,8 +285,40 @@ STVE(stvewx, cpu_stl_data_ra, bswap32, u32)
>>  #undef I
>>  #undef LVE
>>  
>> +#ifdef TARGET_PPC64
>> +#define GET_NB(rb) ((rb >> 56) & 0xFF)
>> +#else
>> +#define GET_NB(rb) ((rb >> 24) & 0xFF)
>> +#endif
>
> A 32-bit VSX implementation seems... improbable.  Simpler to just
> bracket the whole thing with ifdef TARGET_PPC64.

Sure, can be done, just that the instruction specifically got bit 0:7, i
thought that I shouldn't limit my implementation to just PPC64.


>> +
>> +void helper_lxvl(CPUPPCState *env, target_ulong addr,
>> +                 target_ulong xt_num, target_ulong rb)
>
> I think it would be nicer to either have two different helpers for the
> LE and BE cases, or take an endian parameter.  That should allow you
> to share the helper with the lxvll implementation.

Something like the below should work:

#define VSX_LXVL(name, lj)                                              \
void helper_##name(CPUPPCState *env, target_ulong addr,                 \
                   target_ulong xt_num, target_ulong rb)                \
{                                                                       \
    int i;                                                              \
    ppc_vsr_t xt;                                                       \
    uint64_t nb = GET_NB(rb);                                           \
                                                                        \
    xt.s128 = int128_zero();                                            \
    if (nb) {                                                           \
        nb = (nb >= 16) ? 16 : nb;                                      \
        if (msr_le && !lj) {                                            \
            for (i = 16; i > 16 - nb; i--) {                            \
                xt.VsrB(i - 1) = cpu_ldub_data_ra(env, addr, GETPC());  \
                addr = addr_add(env, addr, 1);                          \
            }                                                           \
        } else {                                                        \
            for (i = 0; i < nb; i++) {                                  \
                xt.VsrB(i) = cpu_ldub_data_ra(env, addr, GETPC());      \
                addr = addr_add(env, addr, 1);                          \
            }                                                           \
        }                                                               \
    }                                                                   \
    putVSR(xt_num, &xt, env);                                           \
}

VSX_LXVL(lxvl, 0)
VSX_LXVL(lxvll, 1)

Regards
Nikunj