Re: [Qemu-devel] [PATCH v3 2/5] target-ppc: improve lxvw4x implementation

[David Gibson]

On Mon, Sep 19, 2016 at 04:19:34PM +1000, David Gibson wrote:
> On Fri, Sep 16, 2016 at 04:21:48PM +0530, Nikunj A Dadhania wrote:
> > Load 8byte at a time and manipulate.
> > 
> > Signed-off-by: Nikunj A Dadhania <address@hidden>
> > ---
> >  target-ppc/helper.h                 |  1 +
> >  target-ppc/mem_helper.c             |  5 +++++
> >  target-ppc/translate/vsx-impl.inc.c | 19 +++++--------------
> >  3 files changed, 11 insertions(+), 14 deletions(-)
> > 
> > diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> > index 966f2ce..9f6705d 100644
> > --- a/target-ppc/helper.h
> > +++ b/target-ppc/helper.h
> > @@ -297,6 +297,7 @@ DEF_HELPER_2(mtvscr, void, env, avr)
> >  DEF_HELPER_3(lvebx, void, env, avr, tl)
> >  DEF_HELPER_3(lvehx, void, env, avr, tl)
> >  DEF_HELPER_3(lvewx, void, env, avr, tl)
> > +DEF_HELPER_1(deposit32x2, i64, i64)
> >  DEF_HELPER_3(stvebx, void, env, avr, tl)
> >  DEF_HELPER_3(stvehx, void, env, avr, tl)
> >  DEF_HELPER_3(stvewx, void, env, avr, tl)
> > diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
> > index 6548715..86e493e 100644
> > --- a/target-ppc/mem_helper.c
> > +++ b/target-ppc/mem_helper.c
> > @@ -285,6 +285,11 @@ STVE(stvewx, cpu_stl_data_ra, bswap32, u32)
> >  #undef I
> >  #undef LVE
> >  
> > +uint64_t helper_deposit32x2(uint64_t x)
> > +{
> > +    return deposit64((x >> 32), 32, 32, (x));
> > +}
> 
> It seems a shame to drop out to a helper for something this simple.
> How hard would it be to implement this.. wordswap, I guess you'd call
> it.. in tcg ops?
> 
> I'm also not particularly fond of the deposit32x2 name, though a
> better one doesn't quickly come to mind.
> 
> > +
> >  #undef HI_IDX
> >  #undef LO_IDX
> >  
> > diff --git a/target-ppc/translate/vsx-impl.inc.c 
> > b/target-ppc/translate/vsx-impl.inc.c
> > index eee6052..df278df 100644
> > --- a/target-ppc/translate/vsx-impl.inc.c
> > +++ b/target-ppc/translate/vsx-impl.inc.c
> > @@ -75,7 +75,6 @@ static void gen_lxvdsx(DisasContext *ctx)
> >  static void gen_lxvw4x(DisasContext *ctx)
> >  {
> >      TCGv EA;
> > -    TCGv_i64 tmp;
> >      TCGv_i64 xth = cpu_vsrh(xT(ctx->opcode));
> >      TCGv_i64 xtl = cpu_vsrl(xT(ctx->opcode));
> >      if (unlikely(!ctx->vsx_enabled)) {
> > @@ -84,22 +83,14 @@ static void gen_lxvw4x(DisasContext *ctx)
> >      }
> >      gen_set_access_type(ctx, ACCESS_INT);
> >      EA = tcg_temp_new();
> > -    tmp = tcg_temp_new_i64();
> >  
> >      gen_addr_reg_index(ctx, EA);
> > -    gen_qemu_ld32u_i64(ctx, tmp, EA);
> > -    tcg_gen_addi_tl(EA, EA, 4);
> > -    gen_qemu_ld32u_i64(ctx, xth, EA);
> > -    tcg_gen_deposit_i64(xth, xth, tmp, 32, 32);
> > -
> > -    tcg_gen_addi_tl(EA, EA, 4);
> > -    gen_qemu_ld32u_i64(ctx, tmp, EA);
> > -    tcg_gen_addi_tl(EA, EA, 4);
> > -    gen_qemu_ld32u_i64(ctx, xtl, EA);
> > -    tcg_gen_deposit_i64(xtl, xtl, tmp, 32, 32);
> > -
> > +    tcg_gen_qemu_ld_i64(xth, EA, ctx->mem_idx, MO_LEQ);
> > +    gen_helper_deposit32x2(xth, xth);
> > +    tcg_gen_addi_tl(EA, EA, 8);
> > +    tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_LEQ);
> > +    gen_helper_deposit32x2(xtl, xtl);

..and I think this is wrong for BE mode.  The deposit32x2 will get the
words in the right order, but the bytes within each word will be wrong
because of the LE mode load on a BE setup.


> >      tcg_temp_free(EA);
> > -    tcg_temp_free_i64(tmp);
> >  }
> >  
> >  #define VSX_STORE_SCALAR(name, operation)                     \
> 



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