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

[Peter Maydell]

On Fri, 4 Feb 2022 at 20:41, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> 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.

Ah, I see. Then for this patch
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM