| Message ID | 20170711175937.23140-5-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 07/11/2017 07:59 AM, Alex Bennée wrote: > if (use_goto_tb(s, dest)) { > tcg_gen_goto_tb(n); > gen_set_pc_im(s, dest); > tcg_gen_exit_tb((uintptr_t)s->tb + n); > + s->is_jmp = DISAS_TB_JUMP; > } else { > gen_set_pc_im(s, dest); > gen_goto_ptr(); > + s->is_jmp = DISAS_JUMP; > } I think DISAS_TB_JUMP is appropriate for both cases. When not using goto_tb, the jump is still static and we still chain to the next TB via goto_ptr. Otherwise, Reviewed-by: Richard Henderson <rth@twiddle.net> r~
Richard Henderson <rth@twiddle.net> writes: > On 07/11/2017 07:59 AM, Alex Bennée wrote: >> if (use_goto_tb(s, dest)) { >> tcg_gen_goto_tb(n); >> gen_set_pc_im(s, dest); >> tcg_gen_exit_tb((uintptr_t)s->tb + n); >> + s->is_jmp = DISAS_TB_JUMP; >> } else { >> gen_set_pc_im(s, dest); >> gen_goto_ptr(); >> + s->is_jmp = DISAS_JUMP; >> } > > I think DISAS_TB_JUMP is appropriate for both cases. When not using > goto_tb, the jump is still static and we still chain to the next TB > via goto_ptr. OK - I guess we need to nail down what the essential difference is between the two. I understood DISAS_TB_JUMP as a static known PC which can be patched in the generated code because we know the two addresses are in the same page - whereas DISAS_JUMP is a "computed" jump although in this case the PC is already known. Does making a distinction between computed and non-computer inter-page jumps make any sense anyway? > > Otherwise, > > Reviewed-by: Richard Henderson <rth@twiddle.net> > > > r~ -- Alex Bennée
On 07/11/2017 09:20 AM, Alex Bennée wrote: > > Richard Henderson <rth@twiddle.net> writes: > >> On 07/11/2017 07:59 AM, Alex Bennée wrote: >>> if (use_goto_tb(s, dest)) { >>> tcg_gen_goto_tb(n); >>> gen_set_pc_im(s, dest); >>> tcg_gen_exit_tb((uintptr_t)s->tb + n); >>> + s->is_jmp = DISAS_TB_JUMP; >>> } else { >>> gen_set_pc_im(s, dest); >>> gen_goto_ptr(); >>> + s->is_jmp = DISAS_JUMP; >>> } >> >> I think DISAS_TB_JUMP is appropriate for both cases. When not using >> goto_tb, the jump is still static and we still chain to the next TB >> via goto_ptr. > > OK - I guess we need to nail down what the essential difference is > between the two. I understood DISAS_TB_JUMP as a static known PC which > can be patched in the generated code because we know the two addresses > are in the same page - whereas DISAS_JUMP is a "computed" jump although > in this case the PC is already known. > > Does making a distinction between computed and non-computer inter-page > jumps make any sense anyway? *shrug* probably not. Honestly, the only thing that's really interesting here is that is_jmp indicate that we have already exited the TB and that all following code is dead. As a response to one of Lluis' threads, I suggested that the generic term for this be DISAS_NORETURN. Which also covers exiting the TB via calling a helper that does not return, e.g. throwing an illegal opcode exception, aka DISAS_EXC in the current target/arm sources. r~
diff --git a/target/arm/translate.c b/target/arm/translate.c index 5f2cea8641..82341ee709 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -4158,15 +4158,20 @@ static void gen_goto_ptr(void) tcg_temp_free(addr); } +/* This will end the TB but doesn't guarantee we'll return to + * cpu_loop_exec. Any live exit_requests will be processed as we + * enter the next TB. */ static void gen_goto_tb(DisasContext *s, int n, target_ulong dest) { if (use_goto_tb(s, dest)) { tcg_gen_goto_tb(n); gen_set_pc_im(s, dest); tcg_gen_exit_tb((uintptr_t)s->tb + n); + s->is_jmp = DISAS_TB_JUMP; } else { gen_set_pc_im(s, dest); gen_goto_ptr(); + s->is_jmp = DISAS_JUMP; } } @@ -4179,7 +4184,6 @@ static inline void gen_jmp (DisasContext *s, uint32_t dest) gen_bx_im(s, dest); } else { gen_goto_tb(s, 0, dest); - s->is_jmp = DISAS_TB_JUMP; } }
As the gen_goto_tb function can do both static and dynamic jumps it should also set the is_jmp field. This matches the behaviour of the a64 code. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- target/arm/translate.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)