Re: [Qemu-devel] [PATCH 04/11] target-mips: convert bitfield ops to TCG

[Aurelien Jarno]

On Sat, Nov 08, 2008 at 01:57:21PM +0100, Laurent Desnogues wrote:
> On Sat, Nov 8, 2008 at 9:34 AM, Aurelien Jarno <address@hidden> wrote:
> > Bitfield operations can be written with very few TCG instructions
> > (between 2 and 5), so it is worth converting them to TCG.
> >
> > Signed-off-by: Aurelien Jarno <address@hidden>
> > ---
> >  target-mips/helper.h    |    6 +----
> >  target-mips/op_helper.c |   26 +------------------------
> >  target-mips/translate.c |   49 
> > +++++++++++++++++++++++++++++++++++++---------
> >  3 files changed, 41 insertions(+), 40 deletions(-)
> [...]
> > diff --git a/target-mips/translate.c b/target-mips/translate.c
> > index af01f73..2cd1868 100644
> > --- a/target-mips/translate.c
> > +++ b/target-mips/translate.c
> > @@ -2682,57 +2682,86 @@ static void gen_compute_branch (DisasContext *ctx, 
> > uint32_t opc,
> >  static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt,
> >                         int rs, int lsb, int msb)
> >  {
> > -    TCGv t0 = tcg_temp_local_new(TCG_TYPE_TL);
> > -    TCGv t1 = tcg_temp_local_new(TCG_TYPE_TL);
> > +    TCGv t0 = tcg_temp_new(TCG_TYPE_TL);
> > +    TCGv t1 = tcg_temp_new(TCG_TYPE_TL);
> > +    target_ulong mask;
> >
> >     gen_load_gpr(t1, rs);
> >     switch (opc) {
> >     case OPC_EXT:
> >         if (lsb + msb > 31)
> >             goto fail;
> > -        tcg_gen_helper_1_1ii(do_ext, t0, t1, lsb, msb + 1);
> > +        tcg_gen_shri_tl(t0, t1, lsb);
> > +        if (msb + 1 < 32) {
> 
> Given the above restriction of lsb + msb <= 31, this test can
> be rewritten as:
> 
>      if (msb != 31)
> 
> I find this more readable, but that's personal taste :-)

Fixed.
 
> > +            tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1);
> > +        } else {
> > +            tcg_gen_ext32s_tl(t0, t0);
> > +        }
> >         break;
> >  #if defined(TARGET_MIPS64)
> >     case OPC_DEXTM:
> >         if (lsb + msb > 63)
> >             goto fail;
> 
> Can this really happen?  lsb and msb are 5 bit wide as far
> as I could see.

Not it can't. It comes for the previous code, and I forget to "optimize"
this part.

> > -        tcg_gen_helper_1_1ii(do_dext, t0, t1, lsb, msb + 1 + 32);
> > +        tcg_gen_shri_tl(t0, t1, lsb);
> > +        if (msb + 1 + 32 < 64) {
> 
> This can be rewritten as
> 
>     if (msb != 31)

Fixed.

> > +            tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1 + 32)) - 1);
> > +        }
> >         break;
> >     case OPC_DEXTU:
> >         if (lsb + msb > 63)
> >             goto fail;
> 
> Same as above.

Fixed.

> > -        tcg_gen_helper_1_1ii(do_dext, t0, t1, lsb + 32, msb + 1);
> > +        tcg_gen_shri_tl(t0, t1, lsb + 32);
> > +        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
> >         break;
> >     case OPC_DEXT:
> >         if (lsb + msb > 63)
> >             goto fail;
> 
> Same as above.
> 
Fixed.
> 
> Laurent
> 
> 
> 

-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net