Re: [PATCH v4 3/5] tcg/sparc: Use the constant pool for 64-bit constants

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

On 2/5/22 05:18, Peter Maydell wrote:
> On Fri, 4 Feb 2022 at 07:53, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   tcg/sparc/tcg-target.c.inc | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/tcg/sparc/tcg-target.c.inc b/tcg/sparc/tcg-target.c.inc
>> index 6349f750cc..47bdf314a0 100644
>> --- a/tcg/sparc/tcg-target.c.inc
>> +++ b/tcg/sparc/tcg-target.c.inc
>> @@ -332,6 +332,13 @@ static bool patch_reloc(tcg_insn_unit *src_rw, int type,
>>           insn &= ~INSN_OFF19(-1);
>>           insn |= INSN_OFF19(pcrel);
>>           break;
>> +    case R_SPARC_13:
>> +        if (!check_fit_ptr(value, 13)) {
>> +            return false;
>> +        }
> 
> This code seems to contemplate that the offset might not fit
> into the 13-bit immediate field (unlike the other two reloc
> cases in this function, which just assert() that it fits)...

Ooo, thanks for noticing.  The other entries have not been updated for changes to tcg 
relocations.  They should be returning false instead of asserting.

Returning false here tells generic code the relocation didn't fit, and to restart the TB 
with half of the number of guest instructions.

>> +    /* Use the constant pool, if possible. */
>> +    if (!in_prologue && USE_REG_TB) {
>> +        new_pool_label(s, arg, R_SPARC_13, s->code_ptr,
>> +                       tcg_tbrel_diff(s, NULL));
>> +        tcg_out32(s, LDX | INSN_RD(ret) | INSN_RS1(TCG_REG_TB));
>> +        return;
>> +    }
> 
> ...but this code doesn't seem to have any mechanism for
> falling back to something else if it won't fit.

It will fit, because we'll keep trying with smaller TBs until it does.  If for some reason 
a target generates more than 8k for a single guest insn... it will go boom.


r~