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

[Richard Henderson <richard.henderson@linaro.org>]

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()?


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

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


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


r~