Re: [PATCH v3] target/riscv: Implement dynamic establishment of custom decoder

[Huang Tao <eric.huang@linux.alibaba.com>]

Hi,

On 2024/3/13 18:47, Philippe Mathieu-Daudé wrote:
>
>> +{
>> +    GPtrArray *dynamic_decoders;
>> +    dynamic_decoders = g_ptr_array_sized_new(decoder_table_size);
>> +    for (size_t i = 0; i < decoder_table_size; ++i) {
>> +        if (decoder_table[i].guard_func &&
>> +            decoder_table[i].guard_func(&cpu->cfg)) {
>> +            g_ptr_array_add(dynamic_decoders,
>> + (gpointer)decoder_table[i].decode_fn);
>> +        }
>> +    }
>> +
>> +    cpu->decoders = dynamic_decoders;
>> +}
>
> Move this function to translate.c and make decoder_table[] static.
> Then we don't need the "cpu_decoder.h", it is specific to TCG and
> declarations go in "target/riscv/tcg/tcg-cpu.h".
>
This function is about finalizing the feature of cpu, it is not suitable 
to move it to translate.c from the perspective of code structure and 
readability.

I will try to move the function to tcg-cpu.c, and the declarations to 
tcg-cpu.h according to your suggestion.

>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>> index 177418b2b9..332f0bfd4e 100644
>> --- a/target/riscv/translate.c
>> +++ b/target/riscv/translate.c
>> @@ -115,6 +115,7 @@ typedef struct DisasContext {
>>       bool frm_valid;
>>       /* TCG of the current insn_start */
>>       TCGOp *insn_start;
>> +    const GPtrArray *decoders;
>
> Why do we need this reference? We can use env_archcpu(env)->decoders.
>
As Richard said before:

 > We try to avoid placing env into DisasContext, so that it is much 
harder to make the mistake of referencing env fields at 
translation-time, when you really needed to generate tcg code to 
reference the fields at runtime.

It also applies to the ArchCPU case.


Thanks to your review, I will adopt the other suggestions in the next 
version.


Thanks,

Huang Tao