diff mbox series

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

Message ID 20230503070656.1746170-31-richard.henderson@linaro.org
State Superseded
Headers show
Series tcg: Improve atomicity support | expand

Commit Message

Richard Henderson May 3, 2023, 7:06 a.m. UTC 
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/sparc64/tcg-target.c.inc | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Comments

Peter Maydell May 5, 2023, 12:19 p.m. UTC | #1 
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 ?

thanks
-- PMM
Richard Henderson May 8, 2023, 3:17 p.m. UTC | #2 
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.


r~
Peter Maydell May 9, 2023, 9:24 a.m. UTC | #3 
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.

thanks
-- PMM
Richard Henderson May 9, 2023, 2:34 p.m. UTC | #4 
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~
diff mbox series

Patch

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);
 }
 
 static void tcg_out_ext8s(TCGContext *s, TCGType type, TCGReg rd, TCGReg rs)
@@ -885,10 +885,8 @@  static void tcg_out_jmpl_const(TCGContext *s, const tcg_insn_unit *dest,
 {
     uintptr_t desti = (uintptr_t)dest;
 
-    /* Be careful not to clobber %o7 for a tail call. */
     tcg_out_movi_int(s, TCG_TYPE_PTR, TCG_REG_T1,
-                     desti & ~0xfff, in_prologue,
-                     tail_call ? TCG_REG_G2 : TCG_REG_O7);
+                     desti & ~0xfff, in_prologue, TCG_REG_T2);
     tcg_out_arithi(s, tail_call ? TCG_REG_G0 : TCG_REG_O7,
                    TCG_REG_T1, desti & 0xfff, JMPL);
 }
@@ -1856,6 +1854,7 @@  static void tcg_target_init(TCGContext *s)
     tcg_regset_set_reg(s->reserved_regs, TCG_REG_O6); /* stack pointer */
     tcg_regset_set_reg(s->reserved_regs, TCG_REG_T1); /* for internal use */
     tcg_regset_set_reg(s->reserved_regs, TCG_REG_T2); /* for internal use */
+    tcg_regset_set_reg(s->reserved_regs, TCG_REG_T3); /* for internal use */
 }
 
 #define ELF_HOST_MACHINE  EM_SPARCV9