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

[Daniel Henrique Barboza]

On 3/7/24 17:11, Richard Henderson wrote:
> On 3/7/24 09:55, Daniel Henrique Barboza wrote:
> > (--- adding Richard ---)
> >
> >
> > On 3/6/24 06:33, Huang Tao wrote:
> > > In this patch, we modify the decoder to be a freely composable data
> > > structure instead of a hardcoded one. It can be dynamically builded up
> > > according to the extensions.
> > > This approach has several benefits:
> > > 1. Provides support for heterogeneous cpu architectures. As we add decoder in
> > >     CPUArchState, each cpu can have their own decoder, and the decoders can be
> > >     different due to cpu's features.
> > > 2. Improve the decoding efficiency. We run the guard_func to see if the decoder
> > >     can be added to the dynamic_decoder when building up the decoder. Therefore,
> > >     there is no need to run the guard_func when decoding each instruction. It 
> > > can
> > >     improve the decoding efficiency
> > > 3. For vendor or dynamic cpus, it allows them to customize their own decoder
> > >     functions to improve decoding efficiency, especially when vendor-defined
> > >     instruction sets increase. Because of dynamic building up, it can skip the 
> > > other
> > >     decoder guard functions when decoding.
> > >
> > > Signed-off-by: Huang Tao <eric.huang@linux.alibaba.com>
> > > Suggested-by: Christoph Muellner <christoph.muellner@vrull.eu>
> > > Co-authored-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> > > ---
> > >   target/riscv/cpu.c         | 20 ++++++++++++++++++++
> > >   target/riscv/cpu.h         |  2 ++
> > >   target/riscv/cpu_decoder.h | 34 ++++++++++++++++++++++++++++++++++
> > >   target/riscv/translate.c   | 28 ++++++++++++----------------
> > >   4 files changed, 68 insertions(+), 16 deletions(-)
> > >   create mode 100644 target/riscv/cpu_decoder.h
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 5ff0192c52..cadcd51b4f 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -38,6 +38,7 @@
> > >   #include "kvm/kvm_riscv.h"
> > >   #include "tcg/tcg-cpu.h"
> > >   #include "tcg/tcg.h"
> > > +#include "cpu_decoder.h"
> > >   /* RISC-V CPU definitions */
> > >   static const char riscv_single_letter_exts[] = "IEMAFDQCBPVH";
> > > @@ -1102,6 +1103,23 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, 
> > > Error **errp)
> > >   }
> > >   #endif
> > > +static void riscv_cpu_finalize_dynamic_decoder(RISCVCPU *cpu)
> > > +{
> > > +    CPURISCVState *env = &cpu->env;
> > > +    decode_fn *dynamic_decoder;
> > > +    dynamic_decoder = g_new0(decode_fn, decoder_table_size);
> > > +    int j = 0;
> > > +    for (size_t i = 0; i < decoder_table_size; ++i) {
> > > +        if (decoder_table[i].guard_func &&
> > > +            decoder_table[i].guard_func(&cpu->cfg)) {
> > > +            dynamic_decoder[j] = decoder_table[i].decode_fn;
> > > +            j++;
> > > +        }
> > > +    }
> > > +
> > > +    env->decoder = dynamic_decoder;

In time: I think we should rename this to 'decoders' to make it easier to 
figure out
that it's an array instead of a single element. Same thing with the 
ctx->decoder pointer.

> You should save J into ENV as well, or use GPtrArray which would carry the 
> length along with it naturally.
>
> Is the cpu configuration on each cpu, or on the cpu class?
>
> Even if the configuration is per cpu, this new element belongs in ArchCPU not 
> CPUArchState.  Usually all of CPUArchState is zeroed during reset.
>
> > > @@ -1153,9 +1149,8 @@ static void decode_opc(CPURISCVState *env, DisasContext 
> > > *ctx, uint16_t opcode)
> > >                                                ctx->base.pc_next + 2));
> > >           ctx->opcode = opcode32;
> > > -        for (size_t i = 0; i < ARRAY_SIZE(decoders); ++i) {
> > > -            if (decoders[i].guard_func(ctx->cfg_ptr) &&
> > > -                decoders[i].decode_func(ctx, opcode32)) {
> > > +        for (size_t i = 0; i < decoder_table_size; ++i) {
> > > +            if (ctx->decoder[i](ctx, opcode32)) {
> > >                   return;
>
> You definitely should not be using decoder_table_size here, because you 
> eliminated elements, which are in fact NULL here.
>
> > Am I missing something? It seems we can just remove the ctx->decode pointer 
> > altogether
> > and use env->decoder.
>
> 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.

Right. And you mentioned that ArchState isn't the ideal place and we should put 
the
decoders in ArchCPU, so there's that.

In this case, if we were not to use ctx->decoders, we would need to retrieve a 
RISCVCPU
in riscv_tr_translate_insn() to get access to them ....


I'd rather keep ctx->decoders and assign it in tr_init_disas() since that 
function already
uses a RISCVCPU pointer. The extra pointer in DisasContext seems worthy in this 
case.


Thanks,

Daniel





> r~