Re: [Qemu-devel] [PATCH 7/8] target/ppc: optimize various functions using extract op

[David Gibson]

On Thu, May 11, 2017 at 10:48:42PM -0300, Philippe Mathieu-Daudé wrote:
> Hi Nikunj,
> 
> On 05/11/2017 01:54 AM, Nikunj A Dadhania wrote:
> > Philippe Mathieu-Daudé <address@hidden> writes:
> > 
> > > Applied using Coccinelle script.
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> > > ---
> > >  target/ppc/translate.c              |  9 +++------
> > >  target/ppc/translate/vsx-impl.inc.c | 21 +++++++--------------
> > >  2 files changed, 10 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > index f40b5a1abf..64ab412bf3 100644
> > > --- a/target/ppc/translate.c
> > > +++ b/target/ppc/translate.c
> > > @@ -868,8 +868,7 @@ static inline void gen_op_arith_add(DisasContext 
> > > *ctx, TCGv ret, TCGv arg1,
> > >              }
> > >              tcg_gen_xor_tl(cpu_ca, t0, t1);        /* bits changed w/ 
> > > carry */
> > >              tcg_temp_free(t1);
> > > -            tcg_gen_shri_tl(cpu_ca, cpu_ca, 32);   /* extract bit 32 */
> > > -            tcg_gen_andi_tl(cpu_ca, cpu_ca, 1);
> > > +            tcg_gen_extract_tl(cpu_ca, cpu_ca, 32, 1);
> > >              if (is_isa300(ctx)) {
> > >                  tcg_gen_mov_tl(cpu_ca32, cpu_ca);
> > >              }
> > > @@ -1399,8 +1398,7 @@ static inline void gen_op_arith_subf(DisasContext 
> > > *ctx, TCGv ret, TCGv arg1,
> > >              tcg_temp_free(inv1);
> > >              tcg_gen_xor_tl(cpu_ca, t0, t1);         /* bits changes w/ 
> > > carry */
> > >              tcg_temp_free(t1);
> > > -            tcg_gen_shri_tl(cpu_ca, cpu_ca, 32);    /* extract bit 32 */
> > > -            tcg_gen_andi_tl(cpu_ca, cpu_ca, 1);
> > > +            tcg_gen_extract_tl(cpu_ca, cpu_ca, 32, 1);
> > >              if (is_isa300(ctx)) {
> > >                  tcg_gen_mov_tl(cpu_ca32, cpu_ca);
> > >              }
> > 
> > Above changes are correct.
> > 
> > Rest of them are wrong as discussed above in the thread with Richard.
> >
> 
> I tried to correct the cocci script and ran it again (will post in few min
> as v3) and got:
> 
> $ docker run -it -v `pwd`:`pwd` -w `pwd` petersenna/coccinelle --sp-file
> scripts/coccinelle/tcg_gen_extract.cocci --macro-file
> scripts/cocci-macro-file.h --dir target/ppc
> init_defs_builtins: /usr/lib64/coccinelle/standard.h
> init_defs: scripts/cocci-macro-file.h
> HANDLING: target/ppc/mfrom_table_gen.c
> HANDLING: target/ppc/user_only_helper.c
> HANDLING: target/ppc/mmu-hash64.c
> HANDLING: target/ppc/timebase_helper.c
> HANDLING: target/ppc/gdbstub.c
> HANDLING: target/ppc/translate.c
> candidate at target/ppc/translate.c:5386
>   op_size: tl/tl (same)
>   low_bits: 4 (value: 0xf)
>   len: 0xf
>   len_bits == low_bits
>   candidate IS optimizable
> 
> candidate at target/ppc/translate.c:871
>   op_size: tl/tl (same)
>   low_bits: 1 (value: 0x1)
>   len: 0x1
>   len_bits == low_bits
>   candidate IS optimizable
> 
> candidate at target/ppc/translate.c:1402
>   op_size: tl/tl (same)
>   low_bits: 1 (value: 0x1)
>   len: 0x1
>   len_bits == low_bits
>   candidate IS optimizable
> 
> > > @@ -5383,8 +5381,7 @@ static void gen_mfsri(DisasContext *ctx)
> > >      CHK_SV;
> > >      t0 = tcg_temp_new();
> > >      gen_addr_reg_index(ctx, t0);
> > > -    tcg_gen_shri_tl(t0, t0, 28);
> > > -    tcg_gen_andi_tl(t0, t0, 0xF);
> > > +    tcg_gen_extract_tl(t0, t0, 28, 0xF);
> > >      gen_helper_load_sr(cpu_gpr[rd], cpu_env, t0);
> > >      tcg_temp_free(t0);
> > >      if (ra != 0 && ra != rd)
> 
> 0xF = 0b1111 so this one seems correct to, right?

No, I don't think so.  AFAICT tcg_gen_extract_tl() takes a field
width, not a mask as the last parameter.  So this would need to be
        tcg_gen_extract_tl(t0, t0, 28, 4);

Your script needs to do essentially a log-base-2 of the mask.  I don't
know if Coccinelle can do that..

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