| Message ID | 20210817211803.283639-5-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/riscv: Use tcg_constant_* | expand |
On Wed, Aug 18, 2021 at 5:22 AM Richard Henderson <richard.henderson@linaro.org> 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 <richard.henderson@linaro.org> > --- > 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 *ctx = container_of(dcbase, DisasContext, base); > + > + ctx->zero = tcg_constant_tl(0); This is better to be done in riscv_tr_init_disas_context() where ctx members are initialized. > } > > 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->temp[i]); > + ctx->temp[i] = NULL; > + } > + ctx->ntemp = 0; > > if (ctx->base.is_jmp == DISAS_NEXT) { > target_ulong page_start; > @@ -997,7 +1059,7 @@ static const TranslatorOps riscv_tr_ops = { > > 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() > > translator_loop(&riscv_tr_ops, &ctx.base, cs, tb, max_insns); > } > -- Regards, Bin
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~
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~
On Wed, Aug 18, 2021 at 7:23 AM Richard Henderson <richard.henderson@linaro.org> 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 <richard.henderson@linaro.org> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> 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); > + > + 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->temp[i]); > + ctx->temp[i] = NULL; > + } > + ctx->ntemp = 0; > > if (ctx->base.is_jmp == DISAS_NEXT) { > target_ulong page_start; > @@ -997,7 +1059,7 @@ static const TranslatorOps riscv_tr_ops = { > > void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns) > { > - DisasContext ctx; > + DisasContext ctx = { }; > > translator_loop(&riscv_tr_ops, &ctx.base, cs, tb, max_insns); > } > -- > 2.25.1 > >
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->temp[i]); + ctx->temp[i] = NULL; + } + ctx->ntemp = 0; if (ctx->base.is_jmp == DISAS_NEXT) { target_ulong page_start; @@ -997,7 +1059,7 @@ static const TranslatorOps riscv_tr_ops = { void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns) { - DisasContext ctx; + DisasContext ctx = { }; translator_loop(&riscv_tr_ops, &ctx.base, cs, tb, max_insns); }
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 <richard.henderson@linaro.org> --- target/riscv/translate.c | 102 +++++++++++++++++++++++++++++++-------- 1 file changed, 82 insertions(+), 20 deletions(-) -- 2.25.1