On Sun, 20 Jun 2021 at 23:18, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.hender...@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
