target/riscv/cpu.c | 18 ++++++++++++++++++ target/riscv/cpu.h | 2 ++ target/riscv/cpu_decoder.h | 34 ++++++++++++++++++++++++++++++++++ target/riscv/translate.c | 29 +++++++++++++---------------- 4 files changed, 67 insertions(+), 16 deletions(-) create mode 100644 target/riscv/cpu_decoder.h
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 deocder.
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>
---
Changes in v3:
- use GPtrArray to save decode function poionter list.
---
target/riscv/cpu.c | 18 ++++++++++++++++++
target/riscv/cpu.h | 2 ++
target/riscv/cpu_decoder.h | 34 ++++++++++++++++++++++++++++++++++
target/riscv/translate.c | 29 +++++++++++++----------------
4 files changed, 67 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..9aedd93cf6 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,21 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
}
#endif
+static void riscv_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].decode_fn);
+ }
+ }
+
+ cpu->decoders = dynamic_decoders;
+}
+
void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
{
Error *local_err = NULL;
@@ -1127,6 +1143,8 @@ void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
return;
}
}
+
+ riscv_cpu_finalize_dynamic_decoder(cpu);
}
static void riscv_cpu_realize(DeviceState *dev, Error **errp)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 5d291a7092..923721e67a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -30,6 +30,7 @@
#include "qemu/int128.h"
#include "cpu_bits.h"
#include "cpu_cfg.h"
+#include "cpu_decoder.h"
#include "qapi/qapi-types-common.h"
#include "cpu-qom.h"
@@ -457,6 +458,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/cpu_decoder.h b/target/riscv/cpu_decoder.h
new file mode 100644
index 0000000000..549414ce4c
--- /dev/null
+++ b/target/riscv/cpu_decoder.h
@@ -0,0 +1,34 @@
+/*
+ * QEMU RISC-V CPU Decoder
+ *
+ * Copyright (c) 2023-2024 Alibaba Group
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef RISCV_CPU_DECODER_H
+#define RISCV_CPU_DECODER_H
+
+struct DisasContext;
+struct RISCVCPUConfig;
+typedef struct RISCVDecoder {
+ bool (*guard_func)(const struct RISCVCPUConfig *);
+ bool (*decode_fn)(struct DisasContext *, uint32_t);
+} RISCVDecoder;
+
+typedef bool (*decode_fn)(struct DisasContext *, uint32_t);
+
+extern const size_t decoder_table_size;
+
+extern const RISCVDecoder decoder_table[];
+#endif
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;
} DisasContext;
static inline bool has_ext(DisasContext *ctx, uint32_t ext)
@@ -1118,21 +1119,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 */
@@ -1153,9 +1149,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) {
+ decode_fn func = g_ptr_array_index(ctx->decoders, i);
+ if (func(ctx, opcode32)) {
return;
}
}
@@ -1199,6 +1195,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.0On 3/12/24 23:57, 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 > 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 deocder. > > 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> > --- > > Changes in v3: > - use GPtrArray to save decode function poionter list. > --- > target/riscv/cpu.c | 18 ++++++++++++++++++ > target/riscv/cpu.h | 2 ++ > target/riscv/cpu_decoder.h | 34 ++++++++++++++++++++++++++++++++++ > target/riscv/translate.c | 29 +++++++++++++---------------- > 4 files changed, 67 insertions(+), 16 deletions(-) > create mode 100644 target/riscv/cpu_decoder.h Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Hi,
On 13/3/24 10:57, 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
> 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 deocder.
Typo "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>
> ---
>
> Changes in v3:
> - use GPtrArray to save decode function poionter list.
> ---
> target/riscv/cpu.c | 18 ++++++++++++++++++
> target/riscv/cpu.h | 2 ++
> target/riscv/cpu_decoder.h | 34 ++++++++++++++++++++++++++++++++++
> target/riscv/translate.c | 29 +++++++++++++----------------
> 4 files changed, 67 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..9aedd93cf6 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,21 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> }
> #endif
>
> +static void riscv_cpu_finalize_dynamic_decoder(RISCVCPU *cpu)
riscv_tcg_cpu_finalize_dynamic_decoder()
> +{
> + 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".
> +
> void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> {
> Error *local_err = NULL;
> @@ -1127,6 +1143,8 @@ void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> return;
> }
> }
> +
> + riscv_cpu_finalize_dynamic_decoder(cpu);
Move within the 'if tcg_enabled' block.
> }
>
> static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 5d291a7092..923721e67a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -30,6 +30,7 @@
> #include "qemu/int128.h"
> #include "cpu_bits.h"
> #include "cpu_cfg.h"
> +#include "cpu_decoder.h"
> #include "qapi/qapi-types-common.h"
> #include "cpu-qom.h"
>
> @@ -457,6 +458,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/cpu_decoder.h b/target/riscv/cpu_decoder.h
> new file mode 100644
> index 0000000000..549414ce4c
> --- /dev/null
> +++ b/target/riscv/cpu_decoder.h
> @@ -0,0 +1,34 @@
> +/*
> + * QEMU RISC-V CPU Decoder
> + *
> + * Copyright (c) 2023-2024 Alibaba Group
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef RISCV_CPU_DECODER_H
> +#define RISCV_CPU_DECODER_H
> +
> +struct DisasContext;
> +struct RISCVCPUConfig;
> +typedef struct RISCVDecoder {
> + bool (*guard_func)(const struct RISCVCPUConfig *);
> + bool (*decode_fn)(struct DisasContext *, uint32_t);
> +} RISCVDecoder;
> +
> +typedef bool (*decode_fn)(struct DisasContext *, uint32_t);
> +
> +extern const size_t decoder_table_size;
> +
> +extern const RISCVDecoder decoder_table[];
> +#endif
> 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.
> } DisasContext;
>
> static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> @@ -1118,21 +1119,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 */
> @@ -1153,9 +1149,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) {
> + decode_fn func = g_ptr_array_index(ctx->decoders, i);
> + if (func(ctx, opcode32)) {
> return;
> }
> }
> @@ -1199,6 +1195,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)
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
On 14/3/24 07:24, Huang Tao wrote:
> 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.
Right, he told me the same on IRC recently ;)
>
> It also applies to the ArchCPU case.
>
>
> Thanks to your review, I will adopt the other suggestions in the next
> version.
>
>
> Thanks,
>
> Huang Tao
>
>
>
© 2016 - 2024 Red Hat, Inc.