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

[David Gibson]

On Mon, Dec 12, 2016 at 09:31:11AM +0530, Nikunj Dadhania wrote:
> On 12 December 2016 at 06:00, David Gibson <address@hidden> wrote:
> > 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 <address@hidden>
> >> ---
> >>  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]
> 
> I am not getting you, I am getting index from user. So in case of BE host:
> 
> ext_index = index;
> 
> LE Host:
> 
> ext_index = (16 - index) - es;
> 
> I am already doing that. Am I missing something.

Duh, sorry, apparently I'm blind and missed that logic.

> >> +#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?
> 
> I havent had a change to run on the real hardware, but on the system
> simulator, it happily
> returns extracted content even if UIMM > 12.

Hm.  Returns what exactly?

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