Re: [RFC PATCH] tcg/ppc: Enable direct branching tcg_out_goto_tb with TCG_REG_TB

[Richard Henderson]

On 8/14/23 22:01, Jordan Niethe wrote:
> Direct branch patching was disabled when using TCG_REG_TB in commit
> 736a1588c1 ("tcg/ppc: Fix race in goto_tb implementation"). Commit
> 7502f89c74 ("tcg/ppc: Use prefixed instructions for tcg_out_goto_tb")
> used the support for pc relative ISAv3.1 instructions to re-enable
> direct branch patching on POWER10.
>
> The issue with direct branch patching with TCG_REG_TB is the lack of
> synchronization between the new TCG_REG_TB being established and the
> direct branch being patched in.
>
> If each translation block is responsible for establishing its own
> TCG_REG_TB then there can be no synchronization issue.

That's a good idea, and can be used for other things...

It also begs the question of whether power10 should continue to use TCG_REG_TB, loading 
the address with PADDI.  Or whether power9 should, like power10, disable USE_REG_TB and 
use ADDPCIS throughout.

I imagine it depends on usage frequency, whether use of TCG_REG_TB allows 1 insn, where 
addpcis requires 2 insns and prefixed insns require 2 or 3 insn slots (depending on 
alignment).


> +        tcg_out32(s, MFSPR | RT(TCG_REG_TMP1) | LR);
> +        /* bcl 20,31,$+4 (Preferred form for getting nia.) */
> +        tcg_out32(s, BC | BO_ALWAYS | BI(7, CR_SO) | 0x4 | LK);
> +        tcg_out32(s, MFSPR | RT(TCG_REG_TB) | LR);
> +        tcg_out32(s, ADDI | TAI(TCG_REG_TB, TCG_REG_TB, -8));
> +        tcg_out32(s, MTSPR | RS(TCG_REG_TMP1) | LR);

Don't need to save/restore LR.  It is saved in the prologue and may be clobbered within 
the tb itself (as we do for calls).

> @@ -2678,6 +2693,12 @@ static void tcg_out_goto_tb(TCGContext *s, int which)
>       tcg_out32(s, MTSPR | RS(TCG_REG_TMP1) | CTR);
>       tcg_out32(s, BCCTR | BO_ALWAYS);
>       set_jmp_reset_offset(s, which);
> +
> +    /* For the unlinked case, need to reset TCG_REG_TB.  */
> +    if (USE_REG_TB) {
> +        tcg_out_movi_int(s, TCG_TYPE_I64, TCG_REG_TB,
> +                         (tcg_target_long)s->code_buf, true);
> +    }
>   }

Actually, we don't.  The only time we arrive here is when an unlinked TB branches to 
itself.  TCG_REG_TB is still valid.

> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index ddfe9a96cb..20698131c2 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -6010,6 +6010,9 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb, 
> uint64_t pc_start)
>           tcg_malloc(sizeof(uint64_t) * s->gen_tb->icount * start_words);
>   
>       num_insns = -1;
> +#ifdef TCG_TARGET_NEED_ENTER_TB
> +    tcg_out_enter_tb(s);
> +#endif

Better would be to not have the ifdef, and add this symbol as an empty function in all 
other tcg backends.

I might play around with this a bit.


r~