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

[Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>]

[-- Attachment #1: Type: text/plain, Size: 4123 bytes --]

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 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

[-- Attachment #2: Type: text/html, Size: 6712 bytes --]