From: Huang Tao <eric.huang@linux.alibaba.com>
To: Alistair Francis <alistair23@gmail.com>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
zhiwei_liu@linux.alibaba.com, dbarboza@ventanamicro.com,
liwei1518@gmail.com, bin.meng@windriver.com,
alistair.francis@wdc.com, palmer@dabbelt.com,
Christoph Muellner <christoph.muellner@vrull.eu>,
Richard Henderson <richard.henderson@linaro.org>
Subject: Re: [PATCH v4] target/riscv: Implement dynamic establishment of custom decoder
Date: Mon, 29 Apr 2024 16:26:34 +0800 [thread overview]
Message-ID: <3312833f-57d0-4365-a6ca-b46902b61832@linux.alibaba.com> (raw)
In-Reply-To: <c9fb4c6e-9eae-4302-82cf-b02eacce6d9d@linux.alibaba.com>
On 2024/4/29 15:58, Huang Tao wrote:
>
> On 2024/4/29 11:51, Alistair Francis wrote:
>> On Thu, Mar 14, 2024 at 7:23 PM Huang Tao
>> <eric.huang@linux.alibaba.com> 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
>>> RISCVCPU, 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.
>>> 4. Pre patch for allowing adding a vendor decoder before
>>> decode_insn32() with minimal
>>> overhead for users that don't need this particular vendor decoder.
>>>
>>> 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>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Do you mind rebasing this on
>> https://github.com/alistair23/qemu/tree/riscv-to-apply.next?
>>
>> Alistair
>
> I will rebase this patch on the latest riscv-to-apply.next.
>
> Thanks
>
I successfully applied this patch to the latest riscv-to-apply.next
branch. I wonder what error you met on applying this patch to
riscv-to-apply.next, so I can fix my patch.
Thanks
>>> ---
>>> Changes in v4:
>>> - fix typo
>>> - rename function
>>> - add 'if tcg_enable()'
>>> - move function to tcg-cpu.c and declarations to tcg-cpu.h
>>>
>>> Changes in v3:
>>> - use GPtrArray to save decode function poionter list.
>>> ---
>>> target/riscv/cpu.c | 1 +
>>> target/riscv/cpu.h | 1 +
>>> target/riscv/tcg/tcg-cpu.c | 15 +++++++++++++++
>>> target/riscv/tcg/tcg-cpu.h | 15 +++++++++++++++
>>> target/riscv/translate.c | 31 +++++++++++++++----------------
>>> 5 files changed, 47 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>> index c160b9216b..17070b82a7 100644
>>> --- a/target/riscv/cpu.c
>>> +++ b/target/riscv/cpu.c
>>> @@ -1132,6 +1132,7 @@ void riscv_cpu_finalize_features(RISCVCPU
>>> *cpu, Error **errp)
>>> error_propagate(errp, local_err);
>>> return;
>>> }
>>> + riscv_tcg_cpu_finalize_dynamic_decoder(cpu);
>>> } else if (kvm_enabled()) {
>>> riscv_kvm_cpu_finalize_features(cpu, &local_err);
>>> if (local_err != NULL) {
>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>> index 3b1a02b944..48e67410e1 100644
>>> --- a/target/riscv/cpu.h
>>> +++ b/target/riscv/cpu.h
>>> @@ -457,6 +457,7 @@ struct ArchCPU {
>>> uint32_t pmu_avail_ctrs;
>>> /* Mapping of events to counters */
>>> GHashTable *pmu_event_ctr_map;
>>> + const GPtrArray *decoders;
>>> };
>>>
>>> /**
>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>>> index ab6db817db..c9ab92ea2f 100644
>>> --- a/target/riscv/tcg/tcg-cpu.c
>>> +++ b/target/riscv/tcg/tcg-cpu.c
>>> @@ -853,6 +853,21 @@ void riscv_tcg_cpu_finalize_features(RISCVCPU
>>> *cpu, Error **errp)
>>> }
>>> }
>>>
>>> +void riscv_tcg_cpu_finalize_dynamic_decoder(RISCVCPU *cpu)
>>> +{
>>> + 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].riscv_cpu_decode_fn);
>>> + }
>>> + }
>>> +
>>> + cpu->decoders = dynamic_decoders;
>>> +}
>>> +
>>> bool riscv_cpu_tcg_compatible(RISCVCPU *cpu)
>>> {
>>> return object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_CPU_HOST)
>>> == NULL;
>>> diff --git a/target/riscv/tcg/tcg-cpu.h b/target/riscv/tcg/tcg-cpu.h
>>> index f7b32417f8..ce94253fe4 100644
>>> --- a/target/riscv/tcg/tcg-cpu.h
>>> +++ b/target/riscv/tcg/tcg-cpu.h
>>> @@ -26,4 +26,19 @@ void riscv_cpu_validate_set_extensions(RISCVCPU
>>> *cpu, Error **errp);
>>> void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp);
>>> bool riscv_cpu_tcg_compatible(RISCVCPU *cpu);
>>>
>>> +struct DisasContext;
>>> +struct RISCVCPUConfig;
>>> +typedef struct RISCVDecoder {
>>> + bool (*guard_func)(const struct RISCVCPUConfig *);
>>> + bool (*riscv_cpu_decode_fn)(struct DisasContext *, uint32_t);
>>> +} RISCVDecoder;
>>> +
>>> +typedef bool (*riscv_cpu_decode_fn)(struct DisasContext *, uint32_t);
>>> +
>>> +extern const size_t decoder_table_size;
>>> +
>>> +extern const RISCVDecoder decoder_table[];
>>> +
>>> +void riscv_tcg_cpu_finalize_dynamic_decoder(RISCVCPU *cpu);
>>> +
>>> #endif
>>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>>> index ea5d52b2ef..bce16d5054 100644
>>> --- a/target/riscv/translate.c
>>> +++ b/target/riscv/translate.c
>>> @@ -37,6 +37,8 @@
>>> #include "exec/helper-info.c.inc"
>>> #undef HELPER_H
>>>
>>> +#include "tcg/tcg-cpu.h"
>>> +
>>> /* global register indices */
>>> static TCGv cpu_gpr[32], cpu_gprh[32], cpu_pc, cpu_vl, cpu_vstart;
>>> static TCGv_i64 cpu_fpr[32]; /* assume F and D extensions */
>>> @@ -117,6 +119,7 @@ typedef struct DisasContext {
>>> bool frm_valid;
>>> /* TCG of the current insn_start */
>>> TCGOp *insn_start;
>>> + const GPtrArray *decoders;
>>> } DisasContext;
>>>
>>> static inline bool has_ext(DisasContext *ctx, uint32_t ext)
>>> @@ -1120,21 +1123,16 @@ static inline int insn_len(uint16_t first_word)
>>> return (first_word & 3) == 3 ? 4 : 2;
>>> }
>>>
>>> +const RISCVDecoder decoder_table[] = {
>>> + { always_true_p, decode_insn32 },
>>> + { has_xthead_p, decode_xthead},
>>> + { has_XVentanaCondOps_p, decode_XVentanaCodeOps},
>>> +};
>>> +
>>> +const size_t decoder_table_size = ARRAY_SIZE(decoder_table);
>>> +
>>> static void decode_opc(CPURISCVState *env, DisasContext *ctx,
>>> uint16_t opcode)
>>> {
>>> - /*
>>> - * A table with predicate (i.e., guard) functions and decoder
>>> functions
>>> - * that are tested in-order until a decoder matches onto the
>>> opcode.
>>> - */
>>> - static const struct {
>>> - bool (*guard_func)(const RISCVCPUConfig *);
>>> - bool (*decode_func)(DisasContext *, uint32_t);
>>> - } decoders[] = {
>>> - { always_true_p, decode_insn32 },
>>> - { has_xthead_p, decode_xthead },
>>> - { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
>>> - };
>>> -
>>> ctx->virt_inst_excp = false;
>>> ctx->cur_insn_len = insn_len(opcode);
>>> /* Check for compressed insn */
>>> @@ -1155,9 +1153,9 @@ 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 (guint i = 0; i < ctx->decoders->len; ++i) {
>>> + riscv_cpu_decode_fn func =
>>> g_ptr_array_index(ctx->decoders, i);
>>> + if (func(ctx, opcode32)) {
>>> return;
>>> }
>>> }
>>> @@ -1202,6 +1200,7 @@ static void
>>> riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>>> ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
>>> ctx->zero = tcg_constant_tl(0);
>>> ctx->virt_inst_excp = false;
>>> + ctx->decoders = cpu->decoders;
>>> }
>>>
>>> static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
>>> --
>>> 2.41.0
>>>
>>>
prev parent reply other threads:[~2024-04-29 8:28 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-14 9:21 [PATCH v4] target/riscv: Implement dynamic establishment of custom decoder Huang Tao
2024-04-02 6:58 ` Huang Tao
2024-04-29 3:41 ` Alistair Francis
2024-04-29 3:51 ` Alistair Francis
2024-04-29 7:58 ` Huang Tao
2024-04-29 8:26 ` Huang Tao [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3312833f-57d0-4365-a6ca-b46902b61832@linux.alibaba.com \
--to=eric.huang@linux.alibaba.com \
--cc=alistair.francis@wdc.com \
--cc=alistair23@gmail.com \
--cc=bin.meng@windriver.com \
--cc=christoph.muellner@vrull.eu \
--cc=dbarboza@ventanamicro.com \
--cc=liwei1518@gmail.com \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=zhiwei_liu@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.