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

[Jordan Niethe <jniethe5@gmail.com>]

On 16/8/23 2:07 am, Richard Henderson wrote:
> 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).

Yes, I agree. Your v3 series looks good, I'll try and get some 
performance numbers with it so we can make a decision about which way to 
go on power10 and power9.

> 
> 
>> +        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.

Ok, I was not sure how that was meant to work.

> 
>> 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.

Thank you for that, adding pcrel support on POWER9 too is really cool.

> 
> 
> r~
>