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

[David Gibson]

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.

> +
> +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.

> +{
> +    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) {
> +            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);
> +}
> +
>  #undef HI_IDX
>  #undef LO_IDX
> +#undef GET_NB
>  
>  void helper_tbegin(CPUPPCState *env)
>  {
> diff --git a/target-ppc/translate/vsx-impl.inc.c 
> b/target-ppc/translate/vsx-impl.inc.c
> index 808ee48..6bd70a0 100644
> --- a/target-ppc/translate/vsx-impl.inc.c
> +++ b/target-ppc/translate/vsx-impl.inc.c
> @@ -240,6 +240,33 @@ VSX_VECTOR_LOAD_STORE(stxv, st_i64, 0)
>  VSX_VECTOR_LOAD_STORE(lxvx, ld_i64, 1)
>  VSX_VECTOR_LOAD_STORE(stxvx, st_i64, 1)
>  
> +#define VSX_VECTOR_LOAD_STORE_LENGTH(name)                      \
> +static void gen_##name(DisasContext *ctx)                       \
> +{                                                               \
> +    TCGv EA, xt;                                                \
> +                                                                \
> +    if (xT(ctx->opcode) < 32) {                                 \
> +        if (unlikely(!ctx->vsx_enabled)) {                      \
> +            gen_exception(ctx, POWERPC_EXCP_VSXU);              \
> +            return;                                             \
> +        }                                                       \
> +    } else {                                                    \
> +        if (unlikely(!ctx->altivec_enabled)) {                  \
> +            gen_exception(ctx, POWERPC_EXCP_VPU);               \
> +            return;                                             \
> +        }                                                       \
> +    }                                                           \
> +    EA = tcg_temp_new();                                        \
> +    xt = tcg_const_tl(xT(ctx->opcode));                         \
> +    gen_set_access_type(ctx, ACCESS_INT);                       \
> +    gen_addr_register(ctx, EA);                                 \
> +    gen_helper_##name(cpu_env, EA, xt, cpu_gpr[rB(ctx->opcode)]); \
> +    tcg_temp_free(EA);                                          \
> +    tcg_temp_free(xt);                                          \
> +}
> +
> +VSX_VECTOR_LOAD_STORE_LENGTH(lxvl)
> +
>  #define VSX_LOAD_SCALAR_DS(name, operation)                       \
>  static void gen_##name(DisasContext *ctx)                         \
>  {                                                                 \
> diff --git a/target-ppc/translate/vsx-ops.inc.c 
> b/target-ppc/translate/vsx-ops.inc.c
> index daf6a56..4940ab2 100644
> --- a/target-ppc/translate/vsx-ops.inc.c
> +++ b/target-ppc/translate/vsx-ops.inc.c
> @@ -10,6 +10,7 @@ GEN_HANDLER_E(lxvw4x, 0x1F, 0x0C, 0x18, 0, PPC_NONE, 
> PPC2_VSX),
>  GEN_HANDLER_E(lxvh8x, 0x1F, 0x0C, 0x19, 0, PPC_NONE,  PPC2_ISA300),
>  GEN_HANDLER_E(lxvb16x, 0x1F, 0x0C, 0x1B, 0, PPC_NONE, PPC2_ISA300),
>  GEN_HANDLER_E(lxvx, 0x1F, 0x0C, 0x08, 0x00000040, PPC_NONE, PPC2_ISA300),
> +GEN_HANDLER_E(lxvl, 0x1F, 0x0D, 0x08, 0, PPC_NONE, PPC2_ISA300),
>  
>  GEN_HANDLER_E(stxsdx, 0x1F, 0xC, 0x16, 0, PPC_NONE, PPC2_VSX),
>  GEN_HANDLER_E(stxsibx, 0x1F, 0xD, 0x1C, 0, PPC_NONE, PPC2_ISA300),

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature