Re: [PATCH v4 30/57] tcg/sparc64: Allocate %g2 as a third temporary

[Richard Henderson]

On 5/9/23 10:24, Peter Maydell wrote:
> On Mon, 8 May 2023 at 16:17, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > On 5/5/23 13:19, Peter Maydell wrote:
> > > On Wed, 3 May 2023 at 08:17, Richard Henderson
> > > <richard.henderson@linaro.org> wrote:
> > > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > > > ---
> > > >    tcg/sparc64/tcg-target.c.inc | 15 +++++++--------
> > > >    1 file changed, 7 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/tcg/sparc64/tcg-target.c.inc b/tcg/sparc64/tcg-target.c.inc
> > > > index e997db2645..64464ab363 100644
> > > > --- a/tcg/sparc64/tcg-target.c.inc
> > > > +++ b/tcg/sparc64/tcg-target.c.inc
> > > > @@ -83,9 +83,10 @@ static const char * const 
> > > > tcg_target_reg_names[TCG_TARGET_NB_REGS] = {
> > > >    #define ALL_GENERAL_REGS     MAKE_64BIT_MASK(0, 32)
> > > >    #define ALL_QLDST_REGS       (ALL_GENERAL_REGS & ~SOFTMMU_RESERVE_REGS)
> > > >
> > > > -/* Define some temporary registers.  T2 is used for constant generation.  */
> > > > +/* Define some temporary registers.  T3 is used for constant generation.  */
> > > >    #define TCG_REG_T1  TCG_REG_G1
> > > > -#define TCG_REG_T2  TCG_REG_O7
> > > > +#define TCG_REG_T2  TCG_REG_G2
> > > > +#define TCG_REG_T3  TCG_REG_O7
> > > >
> > > >    #ifndef CONFIG_SOFTMMU
> > > >    # define TCG_GUEST_BASE_REG TCG_REG_I5
> > > > @@ -110,7 +111,6 @@ static const int tcg_target_reg_alloc_order[] = {
> > > >        TCG_REG_I4,
> > > >        TCG_REG_I5,
> > > >
> > > > -    TCG_REG_G2,
> > > >        TCG_REG_G3,
> > > >        TCG_REG_G4,
> > > >        TCG_REG_G5,
> > > > @@ -492,8 +492,8 @@ static void tcg_out_movi_int(TCGContext *s, TCGType type, 
> > > > TCGReg ret,
> > > >    static void tcg_out_movi(TCGContext *s, TCGType type,
> > > >                             TCGReg ret, tcg_target_long arg)
> > > >    {
> > > > -    tcg_debug_assert(ret != TCG_REG_T2);
> > > > -    tcg_out_movi_int(s, type, ret, arg, false, TCG_REG_T2);
> > > > +    tcg_debug_assert(ret != TCG_REG_T3);
> > > > +    tcg_out_movi_int(s, type, ret, arg, false, TCG_REG_T3);
> > > >    }
> > >
> > > Why do we need to change this usage of TCG_REG_T2 but not
> > > any of the others ?
> >
> > To match the comment above.
>
> To expand, what I mean is "when I'm reviewing this patch, what
> do I need to know in order to know whether any particular
> instance of TCG_REG_T2 should be changed to _T3 or not?".
> All the sites where we *don't* change T2 to T3 are now
> using a different register, so there is presumably some
> logic for how we tell whether that's safe or not. The
> "no behaviour change" option would be to change all of them.

Oh.  Well, we could change none of them, including the comment, and also be correct. 
There is no conflict anywhere.

Only with patch 34 ("Use standard slow path for softmmu") do we first see all three temps 
in use at the same time.  Moreover, when I wrote this patch I thought there would in fact 
be a conflict with the use of tcg_out_movi within the slow path patch.  Then I found I 
needed to split out tcg_out_movi_s32 (patch 33) so that I could avoid the assert altogether.

Clearer?  Or not?


r~