Re: [PATCH 3/3] target/riscv: Minimize the calls to decode_save_opc

[Alistair Francis]

On Sun, Jun 5, 2022 at 9:12 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The set of instructions that require decode_save_opc for
> unwinding is really fairly small -- only insns that can
> raise ILLEGAL_INSN at runtime.  This includes CSR, anything
> that uses a *new* fp rounding mode, and many privileged insns.
>
> Since unwind info is stored as the difference from the
> previous insn, storing a 0 for most insns minimizes the
> size of the unwind info.
>
> Booting a debian kernel image to the missing rootfs panic yields
>
> - gen code size       22226819/1026886656
> + gen code size       21601907/1026886656
>
> on 41k TranslationBlocks, a savings of 610kB or a bit less than 3%.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/translate.c                       | 18 +++++++++---------
>  target/riscv/insn_trans/trans_privileged.c.inc |  4 ++++
>  target/riscv/insn_trans/trans_rvh.c.inc        |  2 ++
>  target/riscv/insn_trans/trans_rvi.c.inc        |  2 ++
>  4 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 6e4bbea1cd..b328a7b2ff 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -204,6 +204,13 @@ static void gen_check_nanbox_s(TCGv_i64 out, TCGv_i64 in)
>      tcg_gen_movcond_i64(TCG_COND_GEU, out, in, t_max, in, t_nan);
>  }
>
> +static void decode_save_opc(DisasContext *ctx)
> +{
> +    assert(ctx->insn_start != NULL);
> +    tcg_set_insn_start_param(ctx->insn_start, 1, ctx->opcode);
> +    ctx->insn_start = NULL;
> +}
> +
>  static void gen_set_pc_imm(DisasContext *ctx, target_ulong dest)
>  {
>      if (get_xl(ctx) == MXL_RV32) {
> @@ -633,6 +640,8 @@ static void gen_set_rm(DisasContext *ctx, int rm)
>          return;
>      }
>
> +    /* The helper may raise ILLEGAL_INSN -- record binv for unwind. */
> +    decode_save_opc(ctx);
>      gen_helper_set_rounding_mode(cpu_env, tcg_constant_i32(rm));
>  }
>
> @@ -1011,13 +1020,6 @@ static uint32_t opcode_at(DisasContextBase *dcbase, 
> target_ulong pc)
>  /* Include decoders for factored-out extensions */
>  #include "decode-XVentanaCondOps.c.inc"
>
> -static inline void decode_save_opc(DisasContext *ctx, target_ulong opc)
> -{
> -    assert(ctx->insn_start != NULL);
> -    tcg_set_insn_start_param(ctx->insn_start, 1, opc);
> -    ctx->insn_start = NULL;
> -}
> -
>  static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t 
> opcode)
>  {
>      /*
> @@ -1034,7 +1036,6 @@ static void decode_opc(CPURISCVState *env, DisasContext 
> *ctx, uint16_t opcode)
>
>      /* Check for compressed insn */
>      if (extract16(opcode, 0, 2) != 3) {
> -        decode_save_opc(ctx, opcode);
>          if (!has_ext(ctx, RVC)) {
>              gen_exception_illegal(ctx);
>          } else {
> @@ -1049,7 +1050,6 @@ static void decode_opc(CPURISCVState *env, DisasContext 
> *ctx, uint16_t opcode)
>          opcode32 = deposit32(opcode32, 16, 16,
>                               translator_lduw(env, &ctx->base,
>                                               ctx->base.pc_next + 2));
> -        decode_save_opc(ctx, opcode32);
>          ctx->opcode = opcode32;
>          ctx->pc_succ_insn = ctx->base.pc_next + 4;
>
> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc 
> b/target/riscv/insn_trans/trans_privileged.c.inc
> index 53613682e8..46f96ad74d 100644
> --- a/target/riscv/insn_trans/trans_privileged.c.inc
> +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> @@ -75,6 +75,7 @@ static bool trans_sret(DisasContext *ctx, arg_sret *a)
>  {
>  #ifndef CONFIG_USER_ONLY
>      if (has_ext(ctx, RVS)) {
> +        decode_save_opc(ctx);
>          gen_helper_sret(cpu_pc, cpu_env);
>          tcg_gen_exit_tb(NULL, 0); /* no chaining */
>          ctx->base.is_jmp = DISAS_NORETURN;
> @@ -90,6 +91,7 @@ static bool trans_sret(DisasContext *ctx, arg_sret *a)
>  static bool trans_mret(DisasContext *ctx, arg_mret *a)
>  {
>  #ifndef CONFIG_USER_ONLY
> +    decode_save_opc(ctx);
>      gen_helper_mret(cpu_pc, cpu_env);
>      tcg_gen_exit_tb(NULL, 0); /* no chaining */
>      ctx->base.is_jmp = DISAS_NORETURN;
> @@ -102,6 +104,7 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
>  static bool trans_wfi(DisasContext *ctx, arg_wfi *a)
>  {
>  #ifndef CONFIG_USER_ONLY
> +    decode_save_opc(ctx);
>      gen_set_pc_imm(ctx, ctx->pc_succ_insn);
>      gen_helper_wfi(cpu_env);
>      return true;
> @@ -113,6 +116,7 @@ static bool trans_wfi(DisasContext *ctx, arg_wfi *a)
>  static bool trans_sfence_vma(DisasContext *ctx, arg_sfence_vma *a)
>  {
>  #ifndef CONFIG_USER_ONLY
> +    decode_save_opc(ctx);
>      gen_helper_tlb_flush(cpu_env);
>      return true;
>  #endif
> diff --git a/target/riscv/insn_trans/trans_rvh.c.inc 
> b/target/riscv/insn_trans/trans_rvh.c.inc
> index cebcb3f8f6..4f8aecddc7 100644
> --- a/target/riscv/insn_trans/trans_rvh.c.inc
> +++ b/target/riscv/insn_trans/trans_rvh.c.inc
> @@ -169,6 +169,7 @@ static bool trans_hfence_gvma(DisasContext *ctx, 
> arg_sfence_vma *a)
>  {
>      REQUIRE_EXT(ctx, RVH);
>  #ifndef CONFIG_USER_ONLY
> +    decode_save_opc(ctx);
>      gen_helper_hyp_gvma_tlb_flush(cpu_env);
>      return true;
>  #endif
> @@ -179,6 +180,7 @@ static bool trans_hfence_vvma(DisasContext *ctx, 
> arg_sfence_vma *a)
>  {
>      REQUIRE_EXT(ctx, RVH);
>  #ifndef CONFIG_USER_ONLY
> +    decode_save_opc(ctx);
>      gen_helper_hyp_tlb_flush(cpu_env);
>      return true;
>  #endif
> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc 
> b/target/riscv/insn_trans/trans_rvi.c.inc
> index f1342f30f8..cf17458022 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -822,6 +822,8 @@ static bool trans_fence_i(DisasContext *ctx, arg_fence_i 
> *a)
>
>  static bool do_csr_post(DisasContext *ctx)
>  {
> +    /* The helper may raise ILLEGAL_INSN -- record binv for unwind. */
> +    decode_save_opc(ctx);
>      /* We may have changed important cpu state -- exit to main loop. */
>      gen_set_pc_imm(ctx, ctx->pc_succ_insn);
>      tcg_gen_exit_tb(NULL, 0);
> --
> 2.34.1
>
>