Re: [PATCH v4 4/9] tcg: pass the destination to tcg_gen_lookup_and_goto_ptr()

[Richard Henderson <richard.henderson@linaro.org>]

On 8/26/26 22:02, Matt Turner wrote:
> tcg_gen_lookup_and_goto_ptr() takes no arguments and emits a call to
> helper_lookup_tb_ptr(), which recovers the destination PC from env by
> calling back into the target through TCGCPUOps::get_tb_cpu_state(). At
> translation time the caller already has the destination PC in a temp, and
> knows the flags, cflags and cs_base any destination it may reach has to
> match, because they are the ones the block being generated was translated
> with.
> 
> Pass both, so that a later patch can use them to look the destination up
> inline. Nothing reads them yet and the generated code does not change.
> 
> The contract on @pc is the whole of the interface: it must hold exactly
> what get_tb_cpu_state() reports as the pc for the destination block. Five
> targets keep their PC in a temp whose value is that pc by construction and
> so can pass it: alpha, loongarch, mips, ppc and s390x. Everything else
> passes NULL and keeps today's behavior.
> 
> For six of those the TB pc is derived and passing the PC temp would be
> wrong: avr's TB pc is the word address doubled, i386's is eip before
> segmentation, riscv masks it to 32 bits when xl is MXL_RV32, hppa derives
> it from the IAQ, hexagon adjusts it inside a hardware loop, and sparc puts
> npc in cs_base. The remaining seven -- arm, m68k, microblaze, or1k, rx, sh4
> and tricore -- look like they could pass it, but I have not convinced
> myself of the contract for them and have nothing to test them with. Each is
> a one-line change for whoever wants it.
> 
> The common entry point takes a TCGTemp rather than a TCGv and reads the
> width from it, because the translators that are built for both values of
> TARGET_LONG_BITS -- arm, s390x, microblaze -- cannot include tcg-op.h.
> tcg-op.h wraps it for everyone else. This is the same split as
> tcg_gen_qemu_ld_*_chk().
> 
> v4: Split out of "tcg: probe the TB jump cache inline instead of calling a
>      helper", which did the API change and the inline probe in one patch.
>      Requested by Richard Henderson.
> 
> Signed-off-by: Matt Turner<mattst88@gmail.com>
> ---

Ok, I was a bit surprised at your claim that only 5 targets qualify, as 
there are plenty that have a simple PC.

However! The subtlety of the interface, that we are asserting that the 
current TranslationBlock flags are still valid, now leads me to think 
that it's a mistake to adjust the current interface.

We need to introduce a new interface to which targets may be migrated. 
This won't be difficult, but it's not entirely trivial.

For instance, target/arm/ has

         case DISAS_UPDATE_NOCHAIN:
             gen_update_pc(dc, curr_insn_len(dc));
             /* fall through */
         case DISAS_JUMP:
             gen_goto_ptr();
             break;

where DISAS_UPDATE_NOCHAIN requires the helper because of state change 
and DISAS_JUMP does not.

This is subtle enough that we probably want to verify that the flags are 
unchanged with --enable-debug-tcg.

Perhaps tcg_gen_goto_jc_{i32,i64,tl)?

BTW:

> --- ./include/tcg/tcg-op.h
> +++ ./include/tcg/tcg-op.h
> @@ -49,6 +49,18 @@ typedef TCGv_i64 TCGv;
>  #error Unhandled TARGET_LONG_BITS value
>  #endif
>  
> +/*
> + * See tcg_gen_lookup_and_goto_ptr_tmp().  @pc may be NULL, for a target
> + * whose guest PC is not directly the key a destination block is found by.
> + * A translator that is built for more than one value of TARGET_LONG_BITS,
> + * and so cannot include this header, calls the _tmp() form directly.
> + */
> +static inline void
> +tcg_gen_lookup_and_goto_ptr(TCGv pc, const TranslationBlock *tb)
> +{
> +    tcg_gen_lookup_and_goto_ptr_tmp(pc ? tcgv_tl_temp(pc) : NULL, tb);
> +}
> +

This needs adjustment.  As we migrate binaries to single-binary, we 
start building bits of code once and stop relying on TARGET_LONG_BITS.
Notice where we include "tcg-op-common.h" instead of "tcg-op.h".

The simplest solution, IMO is to define functions for _i32 and _i64,
as for most everything else, and to have a _tl alias in tcg-op.h.


r~