diff mbox series

[v2,13/23] target/i386: Introduce DISAS_JUMP

Message ID 20220906100932.343523-14-richard.henderson@linaro.org
State Superseded
Headers show
Series target/i386: pc-relative translation blocks | expand

Commit Message

Richard Henderson Sept. 6, 2022, 10:09 a.m. UTC 
Drop the unused dest argument to gen_jr().
Remove most of the calls to gen_jr, and use DISAS_JUMP.
Remove some unused loads of eip for lcall and ljmp.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/translate.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

Comments

Paolo Bonzini Sept. 21, 2022, 12:28 p.m. UTC | #1 
On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Drop the unused dest argument to gen_jr().
> Remove most of the calls to gen_jr, and use DISAS_JUMP.
> Remove some unused loads of eip for lcall and ljmp.

The only use outside i386_tr_tb_stop is here:

static void gen_goto_tb(DisasContext *s, int tb_num, target_ulong eip)
{
    target_ulong pc = s->cs_base + eip;

    if (translator_use_goto_tb(&s->base, pc))  {
        /* jump to same page: we can use a direct jump */
        tcg_gen_goto_tb(tb_num);
        gen_jmp_im(s, eip);
        tcg_gen_exit_tb(s->base.tb, tb_num);
        s->base.is_jmp = DISAS_NORETURN;
    } else {
        /* jump to another page */
        gen_jmp_im(s, eip);
        gen_jr(s);
    }
}

Should it set s->base.is_jmp = DISAS_JUMP instead, so that gen_jr() can be
inlined into i386_tr_tb_stop() and removed completely? If not,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo
Richard Henderson Sept. 21, 2022, 11:27 p.m. UTC | #2 
On 9/21/22 12:28, Paolo Bonzini wrote:
> On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Drop the unused dest argument to gen_jr().
>> Remove most of the calls to gen_jr, and use DISAS_JUMP.
>> Remove some unused loads of eip for lcall and ljmp.
> 
> The only use outside i386_tr_tb_stop is here:
> 
> static void gen_goto_tb(DisasContext *s, int tb_num, target_ulong eip)
> {
>      target_ulong pc = s->cs_base + eip;
> 
>      if (translator_use_goto_tb(&s->base, pc))  {
>          /* jump to same page: we can use a direct jump */
>          tcg_gen_goto_tb(tb_num);
>          gen_jmp_im(s, eip);
>          tcg_gen_exit_tb(s->base.tb, tb_num);
>          s->base.is_jmp = DISAS_NORETURN;
>      } else {
>          /* jump to another page */
>          gen_jmp_im(s, eip);
>          gen_jr(s);
>      }
> }
> 
> Should it set s->base.is_jmp = DISAS_JUMP instead, so that gen_jr() can be
> inlined into i386_tr_tb_stop() and removed completely? If not,

It can't, because of conditional branches which do

    brcond something, L1
    gen_goto_tb
L1
    gen_goto_tb

The first gen_goto_tb can't just fall through, it needs to exit.


r~
diff mbox series

Patch

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index f3c26a9956..1997f8d291 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -135,6 +135,7 @@  typedef struct DisasContext {
 #define DISAS_EOB_ONLY         DISAS_TARGET_0
 #define DISAS_EOB_NEXT         DISAS_TARGET_1
 #define DISAS_EOB_INHIBIT_IRQ  DISAS_TARGET_2
+#define DISAS_JUMP             DISAS_TARGET_3
 
 /* The environment in which user-only runs is constrained. */
 #ifdef CONFIG_USER_ONLY
@@ -222,7 +223,7 @@  STUB_HELPER(wrmsr, TCGv_env env)
 #endif
 
 static void gen_eob(DisasContext *s);
-static void gen_jr(DisasContext *s, TCGv dest);
+static void gen_jr(DisasContext *s);
 static void gen_jmp(DisasContext *s, target_ulong eip);
 static void gen_jmp_tb(DisasContext *s, target_ulong eip, int tb_num);
 static void gen_op(DisasContext *s1, int op, MemOp ot, int d);
@@ -2360,7 +2361,7 @@  static void gen_goto_tb(DisasContext *s, int tb_num, target_ulong eip)
     } else {
         /* jump to another page */
         gen_jmp_im(s, eip);
-        gen_jr(s, s->tmp0);
+        gen_jr(s);
     }
 }
 
@@ -2729,7 +2730,7 @@  static void gen_eob(DisasContext *s)
 }
 
 /* Jump to register */
-static void gen_jr(DisasContext *s, TCGv dest)
+static void gen_jr(DisasContext *s)
 {
     do_gen_eob_worker(s, false, false, true);
 }
@@ -5284,7 +5285,7 @@  static bool disas_insn(DisasContext *s, CPUState *cpu)
             gen_push_v(s, s->T1);
             gen_op_jmp_v(s->T0);
             gen_bnd_jmp(s);
-            gen_jr(s, s->T0);
+            s->base.is_jmp = DISAS_JUMP;
             break;
         case 3: /* lcall Ev */
             if (mod == 3) {
@@ -5305,8 +5306,7 @@  static bool disas_insn(DisasContext *s, CPUState *cpu)
                                       tcg_const_i32(dflag - 1),
                                       tcg_const_i32(s->pc - s->cs_base));
             }
-            tcg_gen_ld_tl(s->tmp4, cpu_env, offsetof(CPUX86State, eip));
-            gen_jr(s, s->tmp4);
+            s->base.is_jmp = DISAS_JUMP;
             break;
         case 4: /* jmp Ev */
             if (dflag == MO_16) {
@@ -5314,7 +5314,7 @@  static bool disas_insn(DisasContext *s, CPUState *cpu)
             }
             gen_op_jmp_v(s->T0);
             gen_bnd_jmp(s);
-            gen_jr(s, s->T0);
+            s->base.is_jmp = DISAS_JUMP;
             break;
         case 5: /* ljmp Ev */
             if (mod == 3) {
@@ -5332,8 +5332,7 @@  static bool disas_insn(DisasContext *s, CPUState *cpu)
                 gen_op_movl_seg_T0_vm(s, R_CS);
                 gen_op_jmp_v(s->T1);
             }
-            tcg_gen_ld_tl(s->tmp4, cpu_env, offsetof(CPUX86State, eip));
-            gen_jr(s, s->tmp4);
+            s->base.is_jmp = DISAS_JUMP;
             break;
         case 6: /* push Ev */
             gen_push_v(s, s->T0);
@@ -6773,7 +6772,7 @@  static bool disas_insn(DisasContext *s, CPUState *cpu)
         /* Note that gen_pop_T0 uses a zero-extending load.  */
         gen_op_jmp_v(s->T0);
         gen_bnd_jmp(s);
-        gen_jr(s, s->T0);
+        s->base.is_jmp = DISAS_JUMP;
         break;
     case 0xc3: /* ret */
         ot = gen_pop_T0(s);
@@ -6781,7 +6780,7 @@  static bool disas_insn(DisasContext *s, CPUState *cpu)
         /* Note that gen_pop_T0 uses a zero-extending load.  */
         gen_op_jmp_v(s->T0);
         gen_bnd_jmp(s);
-        gen_jr(s, s->T0);
+        s->base.is_jmp = DISAS_JUMP;
         break;
     case 0xca: /* lret im */
         val = x86_ldsw_code(env, s);
@@ -8811,6 +8810,9 @@  static void i386_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
         gen_update_eip_cur(dc);
         gen_eob_inhibit_irq(dc, true);
         break;
+    case DISAS_JUMP:
+        gen_jr(dc);
+        break;
     default:
         g_assert_not_reached();
     }