Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/nios2/translate.c | 130 ++++++++++++++++++++-------------------
 1 file changed, 67 insertions(+), 63 deletions(-)

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 31653b7912..06705c894d 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -803,75 +803,72 @@ static void gen_exception(DisasContext *dc, uint32_t excp)
 }
 
 /* generate intermediate code for basic block 'tb'.  */
-void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
+static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
 {
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
     CPUNios2State *env = cs->env_ptr;
-    DisasContext dc1, *dc = &dc1;
-    int num_insns;
-
-    /* Initialize DC */
-
-    dc->base.tb = tb;
-    dc->base.singlestep_enabled = cs->singlestep_enabled;
-    dc->base.is_jmp = DISAS_NEXT;
-    dc->base.pc_first = tb->pc;
-    dc->base.pc_next = tb->pc;
+    target_ulong pc = dc->base.pc_first;
+    int page_insns;
 
     dc->zero    = NULL;
-    dc->pc      = tb->pc;
+    dc->pc      = pc;
     dc->mem_idx = cpu_mmu_index(env, false);
 
-    /* Set up instruction counts */
-    num_insns = 0;
-    if (max_insns > 1) {
-        int page_insns = (TARGET_PAGE_SIZE - (tb->pc & ~TARGET_PAGE_MASK)) / 4;
-        if (max_insns > page_insns) {
-            max_insns = page_insns;
-        }
-    }
+    /* Bound the number of insns to execute to those left on the page.  */
+    page_insns = -(pc | TARGET_PAGE_MASK) / 4;
+    dc->base.max_insns = MIN(page_insns, dc->base.max_insns);
+}
 
-    gen_tb_start(tb);
-    do {
-        tcg_gen_insn_start(dc->pc);
-        num_insns++;
+static void nios2_tr_tb_start(DisasContextBase *db, CPUState *cs)
+{
+}
 
-        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
-            gen_exception(dc, EXCP_DEBUG);
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            dc->pc += 4;
-            break;
-        }
+static void nios2_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
+{
+    tcg_gen_insn_start(dcbase->pc_next);
+}
 
-        if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) {
-            gen_io_start();
-        }
+static bool nios2_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
+                                      const CPUBreakpoint *bp)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
 
-        /* Decode an instruction */
-        handle_instruction(dc, env);
+    gen_exception(dc, EXCP_DEBUG);
+    /*
+     * The address covered by the breakpoint must be included in
+     * [tb->pc, tb->pc + tb->size) in order to for it to be
+     * properly cleared -- thus we increment the PC here so that
+     * the logic setting tb->size below does the right thing.
+     */
+    dc->pc += 4;
+    return true;
+}
 
-        dc->pc += 4;
+static void nios2_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
+    CPUNios2State *env = cs->env_ptr;
 
-        /* Translation stops when a conditional branch is encountered.
-         * Otherwise the subsequent code could get translated several times.
-         * Also stop translation when a page boundary is reached.  This
-         * ensures prefetch aborts occur at the right place.  */
-    } while (!dc->base.is_jmp &&
-             !tcg_op_buf_full() &&
-             num_insns < max_insns);
+    /* Decode an instruction */
+    handle_instruction(dc, env);
+
+    dc->base.pc_next += 4;
+    dc->pc += 4;
+}
+
+static void nios2_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
 
     /* Indicate where the next block should start */
     switch (dc->base.is_jmp) {
-    case DISAS_NEXT:
+    case DISAS_TOO_MANY:
     case DISAS_UPDATE:
         /* Save the current PC back into the CPU register */
         tcg_gen_movi_tl(cpu_R[R_PC], dc->pc);
         tcg_gen_exit_tb(NULL, 0);
         break;
 
-    default:
     case DISAS_JUMP:
         /* The jump will already have updated the PC register */
         tcg_gen_exit_tb(NULL, 0);
@@ -880,25 +877,32 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
     case DISAS_NORETURN:
         /* nothing more to generate */
         break;
+
+    default:
+        g_assert_not_reached();
     }
+}
 
-    /* End off the block */
-    gen_tb_end(tb, num_insns);
+static void nios2_tr_disas_log(const DisasContextBase *dcbase, CPUState *cpu)
+{
+    qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first));
+    log_target_disas(cpu, dcbase->pc_first, dcbase->tb->size);
+}
 
-    /* Mark instruction starts for the final generated instruction */
-    tb->size = dc->pc - tb->pc;
-    tb->icount = num_insns;
+static const TranslatorOps nios2_tr_ops = {
+    .init_disas_context = nios2_tr_init_disas_context,
+    .tb_start           = nios2_tr_tb_start,
+    .insn_start         = nios2_tr_insn_start,
+    .breakpoint_check   = nios2_tr_breakpoint_check,
+    .translate_insn     = nios2_tr_translate_insn,
+    .tb_stop            = nios2_tr_tb_stop,
+    .disas_log          = nios2_tr_disas_log,
+};
 
-#ifdef DEBUG_DISAS
-    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
-        && qemu_log_in_addr_range(tb->pc)) {
-        FILE *logfile = qemu_log_lock();
-        qemu_log("IN: %s\n", lookup_symbol(tb->pc));
-        log_target_disas(cs, tb->pc, dc->pc - tb->pc);
-        qemu_log("\n");
-        qemu_log_unlock(logfile);
-    }
-#endif
+void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
+{
+    DisasContext dc;
+    translator_loop(&nios2_tr_ops, &dc.base, cs, tb, max_insns);
 }
 
 void nios2_cpu_dump_state(CPUState *cs, FILE *f, int flags)
-- 
2.25.1

On Sun, 20 Jun 2021 at 23:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/nios2/translate.c | 130 ++++++++++++++++++++-------------------
>  1 file changed, 67 insertions(+), 63 deletions(-)
>
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
> index 31653b7912..06705c894d 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -803,75 +803,72 @@ static void gen_exception(DisasContext *dc, uint32_t excp)
>  }
>
>  /* generate intermediate code for basic block 'tb'.  */
> -void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
> +static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>  {
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>      CPUNios2State *env = cs->env_ptr;
> -    DisasContext dc1, *dc = &dc1;
> -    int num_insns;
> -
> -    /* Initialize DC */
> -
> -    dc->base.tb = tb;
> -    dc->base.singlestep_enabled = cs->singlestep_enabled;
> -    dc->base.is_jmp = DISAS_NEXT;
> -    dc->base.pc_first = tb->pc;
> -    dc->base.pc_next = tb->pc;
> +    target_ulong pc = dc->base.pc_first;

The local variable doesn't really seem necessary -- you could just
write "dc->pc = dc->base.pc_first" and then use "dc->pc" in the
calculation of page_insns.

> +    int page_insns;
>
>      dc->zero    = NULL;
> -    dc->pc      = tb->pc;
> +    dc->pc      = pc;
>      dc->mem_idx = cpu_mmu_index(env, false);
>
> -    /* Set up instruction counts */
> -    num_insns = 0;
> -    if (max_insns > 1) {
> -        int page_insns = (TARGET_PAGE_SIZE - (tb->pc & ~TARGET_PAGE_MASK)) / 4;
> -        if (max_insns > page_insns) {
> -            max_insns = page_insns;
> -        }
> -    }
> +    /* Bound the number of insns to execute to those left on the page.  */
> +    page_insns = -(pc | TARGET_PAGE_MASK) / 4;
> +    dc->base.max_insns = MIN(page_insns, dc->base.max_insns);
> +}
>
> -    gen_tb_start(tb);
> -    do {
> -        tcg_gen_insn_start(dc->pc);
> -        num_insns++;
> +static void nios2_tr_tb_start(DisasContextBase *db, CPUState *cs)
> +{
> +}
>
> -        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> -            gen_exception(dc, EXCP_DEBUG);
> -            /* The address covered by the breakpoint must be included in
> -               [tb->pc, tb->pc + tb->size) in order to for it to be
> -               properly cleared -- thus we increment the PC here so that
> -               the logic setting tb->size below does the right thing.  */
> -            dc->pc += 4;
> -            break;
> -        }
> +static void nios2_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    tcg_gen_insn_start(dcbase->pc_next);
> +}
>
> -        if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) {
> -            gen_io_start();
> -        }
> +static bool nios2_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
> +                                      const CPUBreakpoint *bp)
> +{
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>
> -        /* Decode an instruction */
> -        handle_instruction(dc, env);
> +    gen_exception(dc, EXCP_DEBUG);
> +    /*
> +     * The address covered by the breakpoint must be included in
> +     * [tb->pc, tb->pc + tb->size) in order to for it to be
> +     * properly cleared -- thus we increment the PC here so that
> +     * the logic setting tb->size below does the right thing.
> +     */
> +    dc->pc += 4;

Don't we need to increment dc->base.pc_next here, not dc->pc? The
generic setting of tb->size in accel/tcg uses "db->pc_next - db->pc_first".

Side note: that comment about "setting tb->size below" seems to have
been copied-and-pasted into most of the front-ends, but it's wrong:
the setting of tb->size is no longer "below" but in the common code.

(More generally, some followup patches rationalizing the use of
dc->pc along the lines of the Arm dc->pc_curr vs dc->base.pc_next
might help. There seems to be a fair amount of code which uses
"dc->pc + 4" that could be using pc_next instead.)

> +    return true;

The arm versions of the breakpoint_check hook also set dc->base.is_jmp
to DISAS_NORETURN.
Are they doing that unnecessarily, or do we need to do that here ?

> +}
>
> -        dc->pc += 4;
> +static void nios2_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
> +    CPUNios2State *env = cs->env_ptr;
>
> -        /* Translation stops when a conditional branch is encountered.
> -         * Otherwise the subsequent code could get translated several times.
> -         * Also stop translation when a page boundary is reached.  This
> -         * ensures prefetch aborts occur at the right place.  */
> -    } while (!dc->base.is_jmp &&
> -             !tcg_op_buf_full() &&
> -             num_insns < max_insns);
> +    /* Decode an instruction */
> +    handle_instruction(dc, env);
> +
> +    dc->base.pc_next += 4;
> +    dc->pc += 4;

This isn't wrong, but I think that a setup like the Arm translator
that does
       dc->pc_curr = s->base.pc_next;
       code = cpu_ldl_code(env, s->base.pc_next);
       s->base.pc_next += 4;
       /* dispatch to handler function here */

would be nicer (dunno whether clearer to do as a single thing or
first to do this conversion and then do a followup patch).

thanks
-- PMM

On 6/28/21 8:57 AM, Peter Maydell wrote:
> On Sun, 20 Jun 2021 at 23:18, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/nios2/translate.c | 130 ++++++++++++++++++++-------------------
>>   1 file changed, 67 insertions(+), 63 deletions(-)
>>
>> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
>> index 31653b7912..06705c894d 100644
>> --- a/target/nios2/translate.c
>> +++ b/target/nios2/translate.c
>> @@ -803,75 +803,72 @@ static void gen_exception(DisasContext *dc, uint32_t excp)
>>   }
>>
>>   /* generate intermediate code for basic block 'tb'.  */
>> -void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
>> +static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>>   {
>> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>>       CPUNios2State *env = cs->env_ptr;
>> -    DisasContext dc1, *dc = &dc1;
>> -    int num_insns;
>> -
>> -    /* Initialize DC */
>> -
>> -    dc->base.tb = tb;
>> -    dc->base.singlestep_enabled = cs->singlestep_enabled;
>> -    dc->base.is_jmp = DISAS_NEXT;
>> -    dc->base.pc_first = tb->pc;
>> -    dc->base.pc_next = tb->pc;
>> +    target_ulong pc = dc->base.pc_first;
> 
> The local variable doesn't really seem necessary -- you could just
> write "dc->pc = dc->base.pc_first" and then use "dc->pc" in the
> calculation of page_insns.
> 
>> +    int page_insns;
>>
>>       dc->zero    = NULL;
>> -    dc->pc      = tb->pc;
>> +    dc->pc      = pc;
>>       dc->mem_idx = cpu_mmu_index(env, false);
>>
>> -    /* Set up instruction counts */
>> -    num_insns = 0;
>> -    if (max_insns > 1) {
>> -        int page_insns = (TARGET_PAGE_SIZE - (tb->pc & ~TARGET_PAGE_MASK)) / 4;
>> -        if (max_insns > page_insns) {
>> -            max_insns = page_insns;
>> -        }
>> -    }
>> +    /* Bound the number of insns to execute to those left on the page.  */
>> +    page_insns = -(pc | TARGET_PAGE_MASK) / 4;
>> +    dc->base.max_insns = MIN(page_insns, dc->base.max_insns);
>> +}
>>
>> -    gen_tb_start(tb);
>> -    do {
>> -        tcg_gen_insn_start(dc->pc);
>> -        num_insns++;
>> +static void nios2_tr_tb_start(DisasContextBase *db, CPUState *cs)
>> +{
>> +}
>>
>> -        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
>> -            gen_exception(dc, EXCP_DEBUG);
>> -            /* The address covered by the breakpoint must be included in
>> -               [tb->pc, tb->pc + tb->size) in order to for it to be
>> -               properly cleared -- thus we increment the PC here so that
>> -               the logic setting tb->size below does the right thing.  */
>> -            dc->pc += 4;
>> -            break;
>> -        }
>> +static void nios2_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>> +{
>> +    tcg_gen_insn_start(dcbase->pc_next);
>> +}
>>
>> -        if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) {
>> -            gen_io_start();
>> -        }
>> +static bool nios2_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
>> +                                      const CPUBreakpoint *bp)
>> +{
>> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>>
>> -        /* Decode an instruction */
>> -        handle_instruction(dc, env);
>> +    gen_exception(dc, EXCP_DEBUG);
>> +    /*
>> +     * The address covered by the breakpoint must be included in
>> +     * [tb->pc, tb->pc + tb->size) in order to for it to be
>> +     * properly cleared -- thus we increment the PC here so that
>> +     * the logic setting tb->size below does the right thing.
>> +     */
>> +    dc->pc += 4;
> 
> Don't we need to increment dc->base.pc_next here, not dc->pc? The
> generic setting of tb->size in accel/tcg uses "db->pc_next - db->pc_first".

Yep, thanks.

>> +    return true;
> 
> The arm versions of the breakpoint_check hook also set dc->base.is_jmp
> to DISAS_NORETURN.
> Are they doing that unnecessarily, or do we need to do that here ?

Here it's done in gen_exception.

>> +    handle_instruction(dc, env);
>> +
>> +    dc->base.pc_next += 4;
>> +    dc->pc += 4;
> 
> This isn't wrong, but I think that a setup like the Arm translator
> that does
>         dc->pc_curr = s->base.pc_next;
>         code = cpu_ldl_code(env, s->base.pc_next);
>         s->base.pc_next += 4;
>         /* dispatch to handler function here */
> 
> would be nicer (dunno whether clearer to do as a single thing or
> first to do this conversion and then do a followup patch).

You're right that advancing pc_next first and using that instead of pc+4 globally would be 
a good clean up.


r~