Re: [Qemu-devel] [PATCH v2 5/6] target-ppc: implement xxextractuw instruction

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6286 bytes --]

On Fri, Dec 09, 2016 at 05:47:24PM +0530, Nikunj A Dadhania wrote:
> xxextractuw: VSX Vector Extract Unsigned Word
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  target-ppc/helper.h                 |  1 +
>  target-ppc/int_helper.c             | 21 +++++++++++++++++++++
>  target-ppc/translate/vsx-impl.inc.c | 27 +++++++++++++++++++++++++++
>  target-ppc/translate/vsx-ops.inc.c  |  5 +++++
>  4 files changed, 54 insertions(+)
> 
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index 4707db4..8b30420 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -540,6 +540,7 @@ DEF_HELPER_2(xvrspip, void, env, i32)
>  DEF_HELPER_2(xvrspiz, void, env, i32)
>  DEF_HELPER_2(xxperm, void, env, i32)
>  DEF_HELPER_2(xxpermr, void, env, i32)
> +DEF_HELPER_4(xxextractuw, void, env, tl, tl, i32)
>  
>  DEF_HELPER_2(efscfsi, i32, env, i32)
>  DEF_HELPER_2(efscfui, i32, env, i32)
> diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c
> index 7989b1f..e3f66ac 100644
> --- a/target-ppc/int_helper.c
> +++ b/target-ppc/int_helper.c
> @@ -2033,6 +2033,27 @@ VEXTRACT(uw, u32)
>  VEXTRACT(d, u64)
>  #undef VEXTRACT
>  
> +void helper_xxextractuw(CPUPPCState *env, target_ulong xtn,
> +                        target_ulong xbn, uint32_t index)
> +{
> +    ppc_vsr_t xt, xb;
> +    size_t es = sizeof(uint32_t);
> +    uint32_t ext_index;
> +
> +    getVSR(xbn, &xb, env);
> +    memset(&xt, 0, sizeof(xt));
> +
> +#if defined(HOST_WORDS_BIGENDIAN)
> +    ext_index = index;
> +    memcpy(&xt.u8[8 - es], &xb.u8[ext_index], es);
> +#else
> +    ext_index = (16 - index) - es;
> +    memcpy(&xt.u8[8], &xb.u8[ext_index], es);

Hm.  So, IIUC, ext_index is the byte element - in IBM numbering - to
start copying from.  But I thought that when we have an LE host, the
IBM byte element ordering is reversed from the actual order in host
memory, so we'd need &xb.u8[16 - ext_index - es]

> +#endif
> +
> +    putVSR(xtn, &xt, env);
> +}
> +
>  #define VEXT_SIGNED(name, element, mask, cast, recast)              \
>  void helper_##name(ppc_avr_t *r, ppc_avr_t *b)                      \
>  {                                                                   \
> diff --git a/target-ppc/translate/vsx-impl.inc.c b/target-ppc/translate/vsx-impl.inc.c
> index 2a17c35..1c40a35 100644
> --- a/target-ppc/translate/vsx-impl.inc.c
> +++ b/target-ppc/translate/vsx-impl.inc.c
> @@ -1180,6 +1180,33 @@ static void gen_xxsldwi(DisasContext *ctx)
>      tcg_temp_free_i64(xtl);
>  }
>  
> +#define VSX_EXTRACT(name)                                       \
> +static void gen_##name(DisasContext *ctx)                       \
> +{                                                               \
> +    TCGv xt, xb;                                                \
> +    TCGv_i32 t0 = tcg_temp_new_i32();                           \
> +    uint8_t uimm = UIMM4(ctx->opcode);                          \
> +                                                                \
> +    if (unlikely(!ctx->vsx_enabled)) {                          \
> +        gen_exception(ctx, POWERPC_EXCP_VSXU);                  \
> +        return;                                                 \
> +    }                                                           \
> +    if (uimm > 12) {                                            \

Throughout the helper you use es == sizeof(uint32_t), but here you
hardcode the assumption of 4 bytes, seems a bit inconsistent.

> +        tcg_gen_movi_i64(cpu_vsrh(xT(ctx->opcode)), 0);         \
> +        tcg_gen_movi_i64(cpu_vsrl(xT(ctx->opcode)), 0);         \
> +        return;                                                 \

So, I know the architecture says it is undefined.  But since you're
testing for the bogus case anyway, why not turn this into an
exception.  That seems like it would be more helpful for debugging the
guest than just setting the result to zero.  Or is this done to match
actual hardware behaviour?

> +    }                                                           \
> +    xt = tcg_const_tl(xT(ctx->opcode));                         \
> +    xb = tcg_const_tl(xB(ctx->opcode));                         \
> +    tcg_gen_movi_i32(t0, uimm);                                 \
> +    gen_helper_##name(cpu_env, xt, xb, t0);                     \
> +    tcg_temp_free(xb);                                          \
> +    tcg_temp_free(xt);                                          \
> +    tcg_temp_free_i32(t0);                                      \
> +}
> +
> +VSX_EXTRACT(xxextractuw)
> +
>  #undef GEN_XX2FORM
>  #undef GEN_XX3FORM
>  #undef GEN_XX2IFORM
> diff --git a/target-ppc/translate/vsx-ops.inc.c b/target-ppc/translate/vsx-ops.inc.c
> index 46b95e3..473d925 100644
> --- a/target-ppc/translate/vsx-ops.inc.c
> +++ b/target-ppc/translate/vsx-ops.inc.c
> @@ -49,6 +49,10 @@ GEN_HANDLER2_E(name, #name, 0x3C, opc2 | 1, opc3, 0, PPC_NONE, fl2)
>  GEN_HANDLER2_E(name, #name, 0x3C, opc2 | 0, opc3, 0, PPC_NONE, fl2), \
>  GEN_HANDLER2_E(name, #name, 0x3C, opc2 | 1, opc3, 0, PPC_NONE, fl2)
>  
> +#define GEN_XX2FORM_EXT(name, opc2, opc3, fl2)                          \
> +GEN_HANDLER2_E(name, #name, 0x3C, opc2 | 0, opc3, 0x00100000, PPC_NONE, fl2), \
> +GEN_HANDLER2_E(name, #name, 0x3C, opc2 | 1, opc3, 0x00100000, PPC_NONE, fl2)
> +
>  #define GEN_XX2FORM_EO(name, opc2, opc3, opc4, fl2)                          \
>  GEN_HANDLER2_E_2(name, #name, 0x3C, opc2 | 0, opc3, opc4, 0, PPC_NONE, fl2), \
>  GEN_HANDLER2_E_2(name, #name, 0x3C, opc2 | 1, opc3, opc4, 0, PPC_NONE, fl2)
> @@ -280,6 +284,7 @@ GEN_XX3FORM(xxpermr, 0x08, 0x07, PPC2_ISA300),
>  GEN_XX2FORM(xxspltw, 0x08, 0x0A, PPC2_VSX),
>  GEN_XX1FORM(xxspltib, 0x08, 0x0B, PPC2_ISA300),
>  GEN_XX3FORM_DM(xxsldwi, 0x08, 0x00),
> +GEN_XX2FORM_EXT(xxextractuw, 0x0A, 0x0A, PPC2_ISA300),
>  
>  #define GEN_XXSEL_ROW(opc3) \
>  GEN_HANDLER2_E(xxsel, "xxsel", 0x3C, 0x18, opc3, 0, PPC_NONE, PPC2_VSX), \

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]