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

[Huang Tao]

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