Re: [Qemu-devel] [PATCH v5 + 1/2] target/aarch64: optimize cross-page direct jumps in softmmu

[Emilio G. Cota]

On Sat, Apr 29, 2017 at 12:30:08 +0200, Richard Henderson wrote:
> On 04/28/2017 09:22 PM, Emilio G. Cota wrote:
> >On Fri, Apr 28, 2017 at 15:17:24 -0400, Emilio G. Cota wrote:
> >>+++ b/target/arm/translate-a64.c
> >>@@ -373,8 +373,7 @@ static inline void gen_goto_tb(DisasContext *s, int n, 
> >>uint64_t dest)
> >>          } else if (s->singlestep_enabled) {
> >>              gen_exception_internal(EXCP_DEBUG);
> >>          } else {
> >>-            tcg_gen_exit_tb(0);
> >>-            s->is_jmp = DISAS_TB_JUMP;
> >
> >I'm not sure about removing this line though. Would it be better to leave it?
> >I can't see how TB_JUMP ends up doing anything in the rest of the file.
> 
> Why not just replace this with
> 
>   s->is_jmp = DISAS_JUMP
> 
> and not emit the lookup_and_goto_ptr here at all?

If we don't emit anything here, we get the error you reported
in the other message (icount whatever in cpu-exec.c:599).

I think this is due to callers assuming get_goto_tb does indeed
generate code, instead of deferring it via is_jmp. For example:

    if (cond < 0x0e) {
        /* genuinely conditional branches */
        TCGLabel *label_match = gen_new_label();
        arm_gen_test_cc(cond, label_match);
        gen_goto_tb(s, 0, s->pc);
        gen_set_label(label_match);
        gen_goto_tb(s, 1, addr);
    } else { [...]

So the simplest solution here seems to just emit the goto_ptr helper
in gen_goto_tb().

Regarding the setting of is_jmp to DISAS_TB_JUMP, after having
looked at the code more closely, I think it shouldn't
be removed, since this is the way we break out of the loop in
gen_intermediate_code(), thereby marking this instruction as the
last of the current TB.

I have updated patch 1/2 accordingly. You can cherry-pick it from:
  https://github.com/cota/qemu/tree/tcg-next-v5+

Thanks,

                Emilio