Re: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[Bruno Piazera Larsen]

On 12/05/2021 15:33, Richard Henderson wrote:

> On
>       5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>       
> > diff --git a/include/exec/helper-proto.h
> >         b/include/exec/helper-proto.h
> >         
> >         index ba100793a7..ce287222ee 100644
> >         
> >         --- a/include/exec/helper-proto.h
> >         
> >         +++ b/include/exec/helper-proto.h
> >         
> >         @@ -38,7 +38,9 @@ dh_ctype(ret) HELPER(name) (dh_ctype(t1),
> >         dh_ctype(t2), dh_ctype(t3), \
> >         
> >           #define IN_HELPER_PROTO
> >         
> >             #include "helper.h"
> >         
> >         +#ifdef CONFIG_TCG
> >         
> >           #include "trace/generated-helpers.h"
> >         
> >         +#endif
> >         
> >           #include "accel/tcg/tcg-runtime.h"
> >         
> >           #include "accel/tcg/plugin-helpers.h"
> >         
> >           
>       
>       Um.. this file is exclusively TCG already.
>       
>       Are you missing some use of helper_foo()?
>       
>     
    A lot of files that we are compiling (mainly mmu-*, excp_helper and
    gdbstub IIRC). We could comb through all of them and remove all
    declarations of helpers and wrap the inclusion of helper-proto
    itself in ifdefs, but it felt unnecessarily long. If it is
    preferable, we can do it.
>       
> > diff --git a/target/ppc/cpu_init.c
> >         b/target/ppc/cpu_init.c
> >         
> >         index d0fa219880..9d72dc49cf 100644
> >         
> >         --- a/target/ppc/cpu_init.c
> >         
> >         +++ b/target/ppc/cpu_init.c
> >         
> >         @@ -1204,11 +1204,13 @@ static void
> >         register_BookE206_sprs(CPUPPCState *env, uint32_t mas_mask,
> >         
> >               /* TLB assist registers */
> >         
> >               /* XXX : not implemented */
> >         
> >               for (i = 0; i < 8; i++) {
> >         
> >         +#ifdef CONFIG_TCG
> >         
> >                   void (*uea_write)(DisasContext *ctx, int sprn, int
> >         gprn) =
> >         
> >                       &spr_write_generic32;
> >         
> >                   if (i == 2 && (mas_mask & (1 << i))
> >         && (env->insns_flags & PPC_64B)) {
> >         
> >                       uea_write = &spr_write_generic;
> >         
> >                   }
> >         
> >         +#endif
> >         
> >       
>       
>       You could move this condition into
>       
>       
> >           if (mas_mask & (1 <<
> >         i)) {
> >         
> >                       spr_register(env, mas_sprn[i], mas_names[i],
> >         
> >                                    SPR_NOACCESS, SPR_NOACCESS,
> >         
> >       
>       
>       ... the call here, so that it all automatically compiles out:
>       
>       
>         spr_register(env, ...,
>       
>                      spr_read_generic,
>       
>                      (i == 2 && (env->insns_flags &
>       PPC_64B)
>       
>                       ? spr_write_generic : spr_write_generic32),
>       
>                      0x00000000);
>       
>       
>     
    Yeah, sounds like a better plan.
> > @@ -8606,7 +8608,9 @@ static void
> >         ppc_cpu_realize(DeviceState *dev, Error **errp)
> >         
> >                   }
> >         
> >               }
> >         
> >           +#ifdef CONFIG_TCG
> >         
> >               create_ppc_opcodes(cpu, &local_err);
> >         
> >         +#endif
> >         
> >               if (local_err != NULL) {
> >         
> >                   error_propagate(errp, local_err);
> >         
> >                   goto unrealize;
> >         
> >       
>       
>       Include the error checking into the ifdef.
>       
>     
    Ah, yeah, good idea
>       
>       
> > @@ -8799,7 +8803,9 @@ static void
> >         ppc_cpu_unrealize(DeviceState *dev)
> >         
> >                 cpu_remove_sync(CPU(cpu));
> >         
> >           +#ifdef CONFIG_TCG
> >         
> >               destroy_ppc_opcodes(cpu);
> >         
> >         +#endif
> >         
> >           }
> >         
> >             static gint ppc_cpu_compare_class_pvr(gconstpointer a,
> >         gconstpointer b)
> >         
> >         @@ -9297,7 +9303,9 @@ static void ppc_cpu_class_init(ObjectClass
> >         *oc, void *data)
> >         
> >               cc->class_by_name = ppc_cpu_class_by_name;
> >         
> >               cc->has_work = ppc_cpu_has_work;
> >         
> >               cc->dump_state = ppc_cpu_dump_state;
> >         
> >         +#ifdef CONFIG_TCG
> >         
> >               cc->dump_statistics = ppc_cpu_dump_statistics;
> >         
> >         +#endif
> >         
> >       
>       
>       We should just drop this entirely.  It's supposedly a generic
>       thing, but only used by ppc.  But even then only with source
>       modification to enable DO_PPC_STATISTICS.  And even then as we
>       convert to decodetree, said statistics will not be collected.
>       
>       
>       We should delete everything from cpu_dump_statistics on down.
>       
>     
    uhm, sure. We can do it, but I think should be left as future
    cleanup once we get disable-tcg working
>       
>       
>       r~
>       
>

--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer