Re: [PATCH v2 04/21] target/riscv: Introduce DisasExtend and new helpers
On Wed, Aug 18, 2021 at 7:23 AM Richard Henderson
wrote:
>
> Introduce get_gpr, dest_gpr, temp_new -- new helpers that do not force
> tcg globals into temps, returning a constant 0 for $zero as source and
> a new temp for $zero as destination.
>
> Introduce ctx->w for simplifying word operations, such as addw.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Alistair Francis
Alistair
> ---
> target/riscv/translate.c | 102 +++
> 1 file changed, 82 insertions(+), 20 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index d540c85a1a..d5cf5e5826 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -39,15 +39,25 @@ static TCGv load_val;
>
> #include "exec/gen-icount.h"
>
> +/*
> + * If an operation is being performed on less than TARGET_LONG_BITS,
> + * it may require the inputs to be sign- or zero-extended; which will
> + * depend on the exact operation being performed.
> + */
> +typedef enum {
> +EXT_NONE,
> +EXT_SIGN,
> +EXT_ZERO,
> +} DisasExtend;
> +
> typedef struct DisasContext {
> DisasContextBase base;
> /* pc_succ_insn points to the instruction following base.pc_next */
> target_ulong pc_succ_insn;
> target_ulong priv_ver;
> -bool virt_enabled;
> +target_ulong misa;
> uint32_t opcode;
> uint32_t mstatus_fs;
> -target_ulong misa;
> uint32_t mem_idx;
> /* Remember the rounding mode encoded in the previous fp instruction,
> which we have already installed into env->fp_status. Or -1 for
> @@ -55,6 +65,8 @@ typedef struct DisasContext {
> to any system register, which includes CSR_FRM, so we do not have
> to reset this known value. */
> int frm;
> +bool w;
> +bool virt_enabled;
> bool ext_ifencei;
> bool hlsx;
> /* vector extension */
> @@ -64,7 +76,10 @@ typedef struct DisasContext {
> uint16_t vlen;
> uint16_t mlen;
> bool vl_eq_vlmax;
> +uint8_t ntemp;
> CPUState *cs;
> +TCGv zero;
> +TCGv temp[4];
> } DisasContext;
>
> static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> @@ -172,27 +187,64 @@ static void gen_goto_tb(DisasContext *ctx, int n,
> target_ulong dest)
> }
> }
>
> -/* Wrapper for getting reg values - need to check of reg is zero since
> - * cpu_gpr[0] is not actually allocated
> +/*
> + * Wrappers for getting reg values.
> + *
> + * The $zero register does not have cpu_gpr[0] allocated -- we supply the
> + * constant zero as a source, and an uninitialized sink as destination.
> + *
> + * Further, we may provide an extension for word operations.
> */
> -static void gen_get_gpr(DisasContext *ctx, TCGv t, int reg_num)
> +static TCGv temp_new(DisasContext *ctx)
> {
> -if (reg_num == 0) {
> -tcg_gen_movi_tl(t, 0);
> -} else {
> -tcg_gen_mov_tl(t, cpu_gpr[reg_num]);
> -}
> +assert(ctx->ntemp < ARRAY_SIZE(ctx->temp));
> +return ctx->temp[ctx->ntemp++] = tcg_temp_new();
> }
>
> -/* Wrapper for setting reg values - need to check of reg is zero since
> - * cpu_gpr[0] is not actually allocated. this is more for safety purposes,
> - * since we usually avoid calling the OP_TYPE_gen function if we see a write
> to
> - * $zero
> - */
> -static void gen_set_gpr(DisasContext *ctx, int reg_num_dst, TCGv t)
> +static TCGv get_gpr(DisasContext *ctx, int reg_num, DisasExtend ext)
> {
> -if (reg_num_dst != 0) {
> -tcg_gen_mov_tl(cpu_gpr[reg_num_dst], t);
> +TCGv t;
> +
> +if (reg_num == 0) {
> +return ctx->zero;
> +}
> +
> +switch (ctx->w ? ext : EXT_NONE) {
> +case EXT_NONE:
> +return cpu_gpr[reg_num];
> +case EXT_SIGN:
> +t = temp_new(ctx);
> +tcg_gen_ext32s_tl(t, cpu_gpr[reg_num]);
> +return t;
> +case EXT_ZERO:
> +t = temp_new(ctx);
> +tcg_gen_ext32u_tl(t, cpu_gpr[reg_num]);
> +return t;
> +}
> +g_assert_not_reached();
> +}
> +
> +static void gen_get_gpr(DisasContext *ctx, TCGv t, int reg_num)
> +{
> +tcg_gen_mov_tl(t, get_gpr(ctx, reg_num, EXT_NONE));
> +}
> +
> +static TCGv __attribute__((unused)) dest_gpr(DisasContext *ctx, int reg_num)
> +{
> +if (reg_num == 0 || ctx->w) {
> +return temp_new(ctx);
> +}
> +return cpu_gpr[reg_num];
> +}
> +
> +static void gen_set_gpr(DisasContext *ctx, int reg_num, TCGv t)
> +{
> +if (reg_num != 0) {
> +if (ctx->w) {
> +tcg_gen_ext32s_tl(cpu_gpr[reg_num], t);
> +} else {
> +tcg_gen_mov_tl(cpu_gpr[reg_num], t);
> +}
> }
> }
>
> @@ -927,8 +979,11 @@ static void riscv_tr_init_disas_context(DisasContextBase
> *dcbase, CPUState *cs)
> ctx->cs = cs;
> }
>
> -static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
> +static void riscv_tr_tb_start(DisasContextBase *dcbase, CPUState *cpu)
> {
> +DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +
Re: [PATCH v2 04/21] target/riscv: Introduce DisasExtend and new helpers
On 8/18/21 12:58 AM, Bin Meng wrote: +TCGv temp[4]; Why is 4? Is it enough? Perhaps a comment here is needed here? It's a round number that will cover three operands plus an extra for address computation. r~
Re: [PATCH v2 04/21] target/riscv: Introduce DisasExtend and new helpers
On 8/18/21 12:58 AM, Bin Meng wrote:
+static void gen_set_gpr(DisasContext *ctx, int reg_num, TCGv t)
+{
+if (reg_num != 0) {
+if (ctx->w) {
+tcg_gen_ext32s_tl(cpu_gpr[reg_num], t);
What about zero extension?
All of the RV64 word instructions sign-extend the result.
void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
{
-DisasContext ctx;
+DisasContext ctx = { };
Why is this change? I believe we should explicitly initialize the ctx
in riscv_tr_init_disas_context()
I considered it easier to zero-init the whole thing here.
r~
Re: [PATCH v2 04/21] target/riscv: Introduce DisasExtend and new helpers
On Wed, Aug 18, 2021 at 5:22 AM Richard Henderson
wrote:
>
> Introduce get_gpr, dest_gpr, temp_new -- new helpers that do not force
> tcg globals into temps, returning a constant 0 for $zero as source and
> a new temp for $zero as destination.
>
> Introduce ctx->w for simplifying word operations, such as addw.
>
> Signed-off-by: Richard Henderson
> ---
> target/riscv/translate.c | 102 +++
> 1 file changed, 82 insertions(+), 20 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index d540c85a1a..d5cf5e5826 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -39,15 +39,25 @@ static TCGv load_val;
>
> #include "exec/gen-icount.h"
>
> +/*
> + * If an operation is being performed on less than TARGET_LONG_BITS,
> + * it may require the inputs to be sign- or zero-extended; which will
> + * depend on the exact operation being performed.
> + */
> +typedef enum {
> +EXT_NONE,
> +EXT_SIGN,
> +EXT_ZERO,
> +} DisasExtend;
> +
> typedef struct DisasContext {
> DisasContextBase base;
> /* pc_succ_insn points to the instruction following base.pc_next */
> target_ulong pc_succ_insn;
> target_ulong priv_ver;
> -bool virt_enabled;
> +target_ulong misa;
> uint32_t opcode;
> uint32_t mstatus_fs;
> -target_ulong misa;
> uint32_t mem_idx;
> /* Remember the rounding mode encoded in the previous fp instruction,
> which we have already installed into env->fp_status. Or -1 for
> @@ -55,6 +65,8 @@ typedef struct DisasContext {
> to any system register, which includes CSR_FRM, so we do not have
> to reset this known value. */
> int frm;
> +bool w;
> +bool virt_enabled;
> bool ext_ifencei;
> bool hlsx;
> /* vector extension */
> @@ -64,7 +76,10 @@ typedef struct DisasContext {
> uint16_t vlen;
> uint16_t mlen;
> bool vl_eq_vlmax;
> +uint8_t ntemp;
> CPUState *cs;
> +TCGv zero;
> +TCGv temp[4];
Why is 4? Is it enough? Perhaps a comment here is needed here?
> } DisasContext;
>
> static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> @@ -172,27 +187,64 @@ static void gen_goto_tb(DisasContext *ctx, int n,
> target_ulong dest)
> }
> }
>
> -/* Wrapper for getting reg values - need to check of reg is zero since
> - * cpu_gpr[0] is not actually allocated
> +/*
> + * Wrappers for getting reg values.
> + *
> + * The $zero register does not have cpu_gpr[0] allocated -- we supply the
> + * constant zero as a source, and an uninitialized sink as destination.
> + *
> + * Further, we may provide an extension for word operations.
> */
> -static void gen_get_gpr(DisasContext *ctx, TCGv t, int reg_num)
> +static TCGv temp_new(DisasContext *ctx)
> {
> -if (reg_num == 0) {
> -tcg_gen_movi_tl(t, 0);
> -} else {
> -tcg_gen_mov_tl(t, cpu_gpr[reg_num]);
> -}
> +assert(ctx->ntemp < ARRAY_SIZE(ctx->temp));
> +return ctx->temp[ctx->ntemp++] = tcg_temp_new();
> }
>
> -/* Wrapper for setting reg values - need to check of reg is zero since
> - * cpu_gpr[0] is not actually allocated. this is more for safety purposes,
> - * since we usually avoid calling the OP_TYPE_gen function if we see a write
> to
> - * $zero
> - */
> -static void gen_set_gpr(DisasContext *ctx, int reg_num_dst, TCGv t)
> +static TCGv get_gpr(DisasContext *ctx, int reg_num, DisasExtend ext)
> {
> -if (reg_num_dst != 0) {
> -tcg_gen_mov_tl(cpu_gpr[reg_num_dst], t);
> +TCGv t;
> +
> +if (reg_num == 0) {
> +return ctx->zero;
> +}
> +
> +switch (ctx->w ? ext : EXT_NONE) {
> +case EXT_NONE:
> +return cpu_gpr[reg_num];
> +case EXT_SIGN:
> +t = temp_new(ctx);
> +tcg_gen_ext32s_tl(t, cpu_gpr[reg_num]);
> +return t;
> +case EXT_ZERO:
> +t = temp_new(ctx);
> +tcg_gen_ext32u_tl(t, cpu_gpr[reg_num]);
> +return t;
> +}
> +g_assert_not_reached();
> +}
> +
> +static void gen_get_gpr(DisasContext *ctx, TCGv t, int reg_num)
> +{
> +tcg_gen_mov_tl(t, get_gpr(ctx, reg_num, EXT_NONE));
> +}
> +
> +static TCGv __attribute__((unused)) dest_gpr(DisasContext *ctx, int reg_num)
> +{
> +if (reg_num == 0 || ctx->w) {
> +return temp_new(ctx);
> +}
> +return cpu_gpr[reg_num];
> +}
> +
> +static void gen_set_gpr(DisasContext *ctx, int reg_num, TCGv t)
> +{
> +if (reg_num != 0) {
> +if (ctx->w) {
> +tcg_gen_ext32s_tl(cpu_gpr[reg_num], t);
What about zero extension?
> +} else {
> +tcg_gen_mov_tl(cpu_gpr[reg_num], t);
> +}
> }
> }
>
> @@ -927,8 +979,11 @@ static void riscv_tr_init_disas_context(DisasContextBase
> *dcbase, CPUState *cs)
> ctx->cs = cs;
> }
>
> -static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
> +static void riscv_tr_tb_start(DisasContextBase *dcbase, CPUState *cpu)
> {
> +DisasContext *c
[PATCH v2 04/21] target/riscv: Introduce DisasExtend and new helpers
Introduce get_gpr, dest_gpr, temp_new -- new helpers that do not force
tcg globals into temps, returning a constant 0 for $zero as source and
a new temp for $zero as destination.
Introduce ctx->w for simplifying word operations, such as addw.
Signed-off-by: Richard Henderson
---
target/riscv/translate.c | 102 +++
1 file changed, 82 insertions(+), 20 deletions(-)
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index d540c85a1a..d5cf5e5826 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -39,15 +39,25 @@ static TCGv load_val;
#include "exec/gen-icount.h"
+/*
+ * If an operation is being performed on less than TARGET_LONG_BITS,
+ * it may require the inputs to be sign- or zero-extended; which will
+ * depend on the exact operation being performed.
+ */
+typedef enum {
+EXT_NONE,
+EXT_SIGN,
+EXT_ZERO,
+} DisasExtend;
+
typedef struct DisasContext {
DisasContextBase base;
/* pc_succ_insn points to the instruction following base.pc_next */
target_ulong pc_succ_insn;
target_ulong priv_ver;
-bool virt_enabled;
+target_ulong misa;
uint32_t opcode;
uint32_t mstatus_fs;
-target_ulong misa;
uint32_t mem_idx;
/* Remember the rounding mode encoded in the previous fp instruction,
which we have already installed into env->fp_status. Or -1 for
@@ -55,6 +65,8 @@ typedef struct DisasContext {
to any system register, which includes CSR_FRM, so we do not have
to reset this known value. */
int frm;
+bool w;
+bool virt_enabled;
bool ext_ifencei;
bool hlsx;
/* vector extension */
@@ -64,7 +76,10 @@ typedef struct DisasContext {
uint16_t vlen;
uint16_t mlen;
bool vl_eq_vlmax;
+uint8_t ntemp;
CPUState *cs;
+TCGv zero;
+TCGv temp[4];
} DisasContext;
static inline bool has_ext(DisasContext *ctx, uint32_t ext)
@@ -172,27 +187,64 @@ static void gen_goto_tb(DisasContext *ctx, int n,
target_ulong dest)
}
}
-/* Wrapper for getting reg values - need to check of reg is zero since
- * cpu_gpr[0] is not actually allocated
+/*
+ * Wrappers for getting reg values.
+ *
+ * The $zero register does not have cpu_gpr[0] allocated -- we supply the
+ * constant zero as a source, and an uninitialized sink as destination.
+ *
+ * Further, we may provide an extension for word operations.
*/
-static void gen_get_gpr(DisasContext *ctx, TCGv t, int reg_num)
+static TCGv temp_new(DisasContext *ctx)
{
-if (reg_num == 0) {
-tcg_gen_movi_tl(t, 0);
-} else {
-tcg_gen_mov_tl(t, cpu_gpr[reg_num]);
-}
+assert(ctx->ntemp < ARRAY_SIZE(ctx->temp));
+return ctx->temp[ctx->ntemp++] = tcg_temp_new();
}
-/* Wrapper for setting reg values - need to check of reg is zero since
- * cpu_gpr[0] is not actually allocated. this is more for safety purposes,
- * since we usually avoid calling the OP_TYPE_gen function if we see a write to
- * $zero
- */
-static void gen_set_gpr(DisasContext *ctx, int reg_num_dst, TCGv t)
+static TCGv get_gpr(DisasContext *ctx, int reg_num, DisasExtend ext)
{
-if (reg_num_dst != 0) {
-tcg_gen_mov_tl(cpu_gpr[reg_num_dst], t);
+TCGv t;
+
+if (reg_num == 0) {
+return ctx->zero;
+}
+
+switch (ctx->w ? ext : EXT_NONE) {
+case EXT_NONE:
+return cpu_gpr[reg_num];
+case EXT_SIGN:
+t = temp_new(ctx);
+tcg_gen_ext32s_tl(t, cpu_gpr[reg_num]);
+return t;
+case EXT_ZERO:
+t = temp_new(ctx);
+tcg_gen_ext32u_tl(t, cpu_gpr[reg_num]);
+return t;
+}
+g_assert_not_reached();
+}
+
+static void gen_get_gpr(DisasContext *ctx, TCGv t, int reg_num)
+{
+tcg_gen_mov_tl(t, get_gpr(ctx, reg_num, EXT_NONE));
+}
+
+static TCGv __attribute__((unused)) dest_gpr(DisasContext *ctx, int reg_num)
+{
+if (reg_num == 0 || ctx->w) {
+return temp_new(ctx);
+}
+return cpu_gpr[reg_num];
+}
+
+static void gen_set_gpr(DisasContext *ctx, int reg_num, TCGv t)
+{
+if (reg_num != 0) {
+if (ctx->w) {
+tcg_gen_ext32s_tl(cpu_gpr[reg_num], t);
+} else {
+tcg_gen_mov_tl(cpu_gpr[reg_num], t);
+}
}
}
@@ -927,8 +979,11 @@ static void riscv_tr_init_disas_context(DisasContextBase
*dcbase, CPUState *cs)
ctx->cs = cs;
}
-static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
+static void riscv_tr_tb_start(DisasContextBase *dcbase, CPUState *cpu)
{
+DisasContext *ctx = container_of(dcbase, DisasContext, base);
+
+ctx->zero = tcg_constant_tl(0);
}
static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
@@ -946,6 +1001,13 @@ static void riscv_tr_translate_insn(DisasContextBase
*dcbase, CPUState *cpu)
decode_opc(env, ctx, opcode16);
ctx->base.pc_next = ctx->pc_succ_insn;
+ctx->w = false;
+
+for (int i = ctx->ntemp - 1; i >= 0; --i) {
+tcg_temp_free(ctx->
