[Qemu-devel] [PATCH] tcp/mips: Change TCG_AREG0 (fp -> s0)

[Qemu-devel] [PATCH] tcp/mips: Change TCG_AREG0 (fp -> s0)

[Stefan Weil]

Register fp is a bad choice for compilations without
optimisation, because the compiler makes heavy use
of this register (so the resulting code crashes).

Register s0 was used for TCG_AREG1 in earlier releases,
but was no longer used and is now free for TCG_AREG0.

The resulting code works for compilations without
optimisation (tested with qemu mips in qemu mips
on x86 host).

Cc: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 dyngen-exec.h         |    2 +-
 tcg/mips/tcg-target.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/dyngen-exec.h b/dyngen-exec.h
index b9f6969..85a2616 100644
--- a/dyngen-exec.h
+++ b/dyngen-exec.h
@@ -61,7 +61,7 @@ extern int printf(const char *, ...);
 #elif defined(__hppa__)
 #define AREG0 "r17"
 #elif defined(__mips__)
-#define AREG0 "fp"
+#define AREG0 "s0"
 #elif defined(__sparc__)
 #ifdef CONFIG_SOLARIS
 #define AREG0 "g2"
diff --git a/tcg/mips/tcg-target.h b/tcg/mips/tcg-target.h
index 0292d33..0028bfa 100644
--- a/tcg/mips/tcg-target.h
+++ b/tcg/mips/tcg-target.h
@@ -97,7 +97,7 @@ enum {
 #undef TCG_TARGET_HAS_ext16u_i32   /* andi rt, rs, 0xffff */
 
 /* Note: must be synced with dyngen-exec.h */
-#define TCG_AREG0 TCG_REG_FP
+#define TCG_AREG0 TCG_REG_S0
 
 /* guest base is supported */
 #define TCG_TARGET_HAS_GUEST_BASE
-- 
1.5.6.5

[Qemu-devel] Re: [PATCH] tcp/mips: Change TCG_AREG0 (fp -> s0)

[Aurelien Jarno]

On Thu, Apr 08, 2010 at 03:38:52PM +0200, Stefan Weil wrote:
> Register fp is a bad choice for compilations without
> optimisation, because the compiler makes heavy use
> of this register (so the resulting code crashes).

I don't fully understand why the compiler makes use of this register in 
code where env is declared as register fp.

> Register s0 was used for TCG_AREG1 in earlier releases,
> but was no longer used and is now free for TCG_AREG0.
> 
> The resulting code works for compilations without
> optimisation (tested with qemu mips in qemu mips
> on x86 host).

The patch is not complete, at least some changes are missing to 
tcg_target_callee_save_regs.

> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  dyngen-exec.h         |    2 +-
>  tcg/mips/tcg-target.h |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/dyngen-exec.h b/dyngen-exec.h
> index b9f6969..85a2616 100644
> --- a/dyngen-exec.h
> +++ b/dyngen-exec.h
> @@ -61,7 +61,7 @@ extern int printf(const char *, ...);
>  #elif defined(__hppa__)
>  #define AREG0 "r17"
>  #elif defined(__mips__)
> -#define AREG0 "fp"
> +#define AREG0 "s0"
>  #elif defined(__sparc__)
>  #ifdef CONFIG_SOLARIS
>  #define AREG0 "g2"
> diff --git a/tcg/mips/tcg-target.h b/tcg/mips/tcg-target.h
> index 0292d33..0028bfa 100644
> --- a/tcg/mips/tcg-target.h
> +++ b/tcg/mips/tcg-target.h
> @@ -97,7 +97,7 @@ enum {
>  #undef TCG_TARGET_HAS_ext16u_i32   /* andi rt, rs, 0xffff */
>  
>  /* Note: must be synced with dyngen-exec.h */
> -#define TCG_AREG0 TCG_REG_FP
> +#define TCG_AREG0 TCG_REG_S0
>  
>  /* guest base is supported */
>  #define TCG_TARGET_HAS_GUEST_BASE
> -- 
> 1.5.6.5
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH] tcp/mips: Change TCG_AREG0 (fp -> s0)

[Paul Brook]

> On Thu, Apr 08, 2010 at 03:38:52PM +0200, Stefan Weil wrote:
> > Register fp is a bad choice for compilations without
> > optimisation, because the compiler makes heavy use
> > of this register (so the resulting code crashes).
> 
> I don't fully understand why the compiler makes use of this register in
> code where env is declared as register fp.

The frame pointer (and certain other registers, typically stack pointer, link 
register, PIC base, TLS base, etc.) are special. They have predefined uses, 
usually specified by the relevant platform ABI.

Defining a global register variable (or using -ffixed-reg) only excludes these 
registers from general register allocation.  It does not prevent them being 
used for specific purposes.  The user is responsible for ensuring that any 
assumptions/requirements made by the ABI are still met.

In some cases gcc can diagnose invalid uses. In others the overlap may be 
legitimate, but not necessarily what we intended in qemu. 

Paul

[Qemu-devel] Re: [PATCH] tcp/mips: Change TCG_AREG0 (fp -> s0)

[Stefan Weil]

Aurelien Jarno schrieb:
> On Thu, Apr 08, 2010 at 03:38:52PM +0200, Stefan Weil wrote:
>   
>> Register fp is a bad choice for compilations without
>> optimisation, because the compiler makes heavy use
>> of this register (so the resulting code crashes).
>>     
>
> I don't fully understand why the compiler makes use of this register in 
> code where env is declared as register fp.
>   

fp = frame pointer is special. See Paul's answer.

>   
>> Register s0 was used for TCG_AREG1 in earlier releases,
>> but was no longer used and is now free for TCG_AREG0.
>>
>> The resulting code works for compilations without
>> optimisation (tested with qemu mips in qemu mips
>> on x86 host).
>>     
>
> The patch is not complete, at least some changes are missing to 
> tcg_target_callee_save_regs.
>   

Thanks. I'll send an updated patch.

Regards, Stefan

[Qemu-devel] [PATCH] tcp/mips: Change TCG_AREG0 (fp -> s0)

[Stefan Weil]

Register fp (frame pointer) is a bad choice for compilations
without optimisation, because the compiler makes heavy use
of this register (so the resulting code crashes).

Register s0 was used for TCG_AREG1 in earlier releases,
but was no longer used and is now free for TCG_AREG0.

The resulting code works for compilations without
optimisation (tested with qemu mips in qemu mips
on x86 host).

v2:

* Remove s0 from tcg_target_callee_save_regs and add fp there.
  Hint from Aurelien Jarno, thanks.

* Add fp to list of reserved registers.

Cc: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 dyngen-exec.h         |    2 +-
 tcg/mips/tcg-target.c |    6 ++++--
 tcg/mips/tcg-target.h |    2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/dyngen-exec.h b/dyngen-exec.h
index b9f6969..85a2616 100644
--- a/dyngen-exec.h
+++ b/dyngen-exec.h
@@ -61,7 +61,7 @@ extern int printf(const char *, ...);
 #elif defined(__hppa__)
 #define AREG0 "r17"
 #elif defined(__mips__)
-#define AREG0 "fp"
+#define AREG0 "s0"
 #elif defined(__sparc__)
 #ifdef CONFIG_SOLARIS
 #define AREG0 "g2"
diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c
index f4fb615..c9094ee 100644
--- a/tcg/mips/tcg-target.c
+++ b/tcg/mips/tcg-target.c
@@ -1450,7 +1450,9 @@ static const TCGTargetOpDef mips_op_defs[] = {
 };
 
 static int tcg_target_callee_save_regs[] = {
+#if 0 /* used for the global env (TCG_AREG0), so no need to save */
     TCG_REG_S0,
+#endif
     TCG_REG_S1,
     TCG_REG_S2,
     TCG_REG_S3,
@@ -1459,8 +1461,7 @@ static int tcg_target_callee_save_regs[] = {
     TCG_REG_S6,
     TCG_REG_S7,
     TCG_REG_GP,
-    /* TCG_REG_FP, */ /* currently used for the global env, so np
-                         need to save */
+    TCG_REG_FP,
     TCG_REG_RA,       /* should be last for ABI compliance */
 };
 
@@ -1524,6 +1525,7 @@ void tcg_target_init(TCGContext *s)
     tcg_regset_set_reg(s->reserved_regs, TCG_REG_AT);   /* internal use */
     tcg_regset_set_reg(s->reserved_regs, TCG_REG_T0);   /* internal use */
     tcg_regset_set_reg(s->reserved_regs, TCG_REG_RA);   /* return address */
+    tcg_regset_set_reg(s->reserved_regs, TCG_REG_FP);   /* frame pointer */
     tcg_regset_set_reg(s->reserved_regs, TCG_REG_SP);   /* stack pointer */
 
     tcg_add_target_add_op_defs(mips_op_defs);
diff --git a/tcg/mips/tcg-target.h b/tcg/mips/tcg-target.h
index 0292d33..0028bfa 100644
--- a/tcg/mips/tcg-target.h
+++ b/tcg/mips/tcg-target.h
@@ -97,7 +97,7 @@ enum {
 #undef TCG_TARGET_HAS_ext16u_i32   /* andi rt, rs, 0xffff */
 
 /* Note: must be synced with dyngen-exec.h */
-#define TCG_AREG0 TCG_REG_FP
+#define TCG_AREG0 TCG_REG_S0
 
 /* guest base is supported */
 #define TCG_TARGET_HAS_GUEST_BASE
-- 
1.7.0

Re: [Qemu-devel] [PATCH] tcp/mips: Change TCG_AREG0 (fp -> s0)

[Richard Henderson]

On 04/09/2010 12:36 AM, Stefan Weil wrote:
> * Add fp to list of reserved registers.

This is unnecessary.  The register is not a frame pointer
within tcg generated code.  It's available for use as a
call-saved register.


r~

[Qemu-devel] [PATCH] tcp/mips: Change TCG_AREG0 (fp -> s0)

[Stefan Weil]

Register fp (frame pointer) is a bad choice for compilations
without optimisation, because the compiler makes heavy use
of this register (so the resulting code crashes).

Register s0 had been used for TCG_AREG1 in earlier releases,
but was no longer used and is now free for TCG_AREG0.

The resulting code works for compilations without
optimisation (tested with qemu mips in qemu mips
on x86 host).

v2:

* Remove s0 from tcg_target_callee_save_regs and add fp there.
  Hint from Aurelien Jarno, thanks.

* Add fp to list of reserved registers.

v3:

* Don't add fp to list of reserved registers
  Thanks to Richard Henderson.

* Remove s0 from tcg_target_reg_alloc_order.

Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 dyngen-exec.h         |    2 +-
 tcg/mips/tcg-target.c |    7 +++++--
 tcg/mips/tcg-target.h |    2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/dyngen-exec.h b/dyngen-exec.h
index d04eda8..0700a2d 100644
--- a/dyngen-exec.h
+++ b/dyngen-exec.h
@@ -59,7 +59,7 @@ extern int printf(const char *, ...);
 #elif defined(__hppa__)
 #define AREG0 "r17"
 #elif defined(__mips__)
-#define AREG0 "fp"
+#define AREG0 "s0"
 #elif defined(__sparc__)
 #ifdef CONFIG_SOLARIS
 #define AREG0 "g2"
diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c
index f4fb615..31296fc 100644
--- a/tcg/mips/tcg-target.c
+++ b/tcg/mips/tcg-target.c
@@ -69,7 +69,9 @@ static const char * const tcg_target_reg_names[TCG_TARGET_NB_REGS] = {
 
 /* check if we really need so many registers :P */
 static const int tcg_target_reg_alloc_order[] = {
+#if 0 /* used for the global env (TCG_AREG0), so don't use it here */
     TCG_REG_S0,
+#endif
     TCG_REG_S1,
     TCG_REG_S2,
     TCG_REG_S3,
@@ -1450,7 +1452,9 @@ static const TCGTargetOpDef mips_op_defs[] = {
 };
 
 static int tcg_target_callee_save_regs[] = {
+#if 0 /* used for the global env (TCG_AREG0), so no need to save */
     TCG_REG_S0,
+#endif
     TCG_REG_S1,
     TCG_REG_S2,
     TCG_REG_S3,
@@ -1459,8 +1463,7 @@ static int tcg_target_callee_save_regs[] = {
     TCG_REG_S6,
     TCG_REG_S7,
     TCG_REG_GP,
-    /* TCG_REG_FP, */ /* currently used for the global env, so np
-                         need to save */
+    TCG_REG_FP,
     TCG_REG_RA,       /* should be last for ABI compliance */
 };
 
diff --git a/tcg/mips/tcg-target.h b/tcg/mips/tcg-target.h
index 0292d33..0028bfa 100644
--- a/tcg/mips/tcg-target.h
+++ b/tcg/mips/tcg-target.h
@@ -97,7 +97,7 @@ enum {
 #undef TCG_TARGET_HAS_ext16u_i32   /* andi rt, rs, 0xffff */
 
 /* Note: must be synced with dyngen-exec.h */
-#define TCG_AREG0 TCG_REG_FP
+#define TCG_AREG0 TCG_REG_S0
 
 /* guest base is supported */
 #define TCG_TARGET_HAS_GUEST_BASE
-- 
1.7.0

[Qemu-devel] Re: [PATCH] tcp/mips: Change TCG_AREG0 (fp -> s0)

[Aurelien Jarno]

On Fri, Apr 09, 2010 at 05:28:40PM +0200, Stefan Weil wrote:
> Register fp (frame pointer) is a bad choice for compilations
> without optimisation, because the compiler makes heavy use
> of this register (so the resulting code crashes).
> 
> Register s0 had been used for TCG_AREG1 in earlier releases,
> but was no longer used and is now free for TCG_AREG0.
> 
> The resulting code works for compilations without
> optimisation (tested with qemu mips in qemu mips
> on x86 host).
> 
> v2:
> 
> * Remove s0 from tcg_target_callee_save_regs and add fp there.
>   Hint from Aurelien Jarno, thanks.
> 
> * Add fp to list of reserved registers.
> 
> v3:
> 
> * Don't add fp to list of reserved registers
>   Thanks to Richard Henderson.
> 
> * Remove s0 from tcg_target_reg_alloc_order.
> 
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  dyngen-exec.h         |    2 +-
>  tcg/mips/tcg-target.c |    7 +++++--
>  tcg/mips/tcg-target.h |    2 +-
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/dyngen-exec.h b/dyngen-exec.h
> index d04eda8..0700a2d 100644
> --- a/dyngen-exec.h
> +++ b/dyngen-exec.h
> @@ -59,7 +59,7 @@ extern int printf(const char *, ...);
>  #elif defined(__hppa__)
>  #define AREG0 "r17"
>  #elif defined(__mips__)
> -#define AREG0 "fp"
> +#define AREG0 "s0"
>  #elif defined(__sparc__)
>  #ifdef CONFIG_SOLARIS
>  #define AREG0 "g2"
> diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c
> index f4fb615..31296fc 100644
> --- a/tcg/mips/tcg-target.c
> +++ b/tcg/mips/tcg-target.c
> @@ -69,7 +69,9 @@ static const char * const tcg_target_reg_names[TCG_TARGET_NB_REGS] = {
>  
>  /* check if we really need so many registers :P */
>  static const int tcg_target_reg_alloc_order[] = {
> +#if 0 /* used for the global env (TCG_AREG0), so don't use it here */
>      TCG_REG_S0,
> +#endif

This is not something necessary. The fact this register is declared as
TCG_AREG0, will make TCG skip this automatically.

>      TCG_REG_S1,
>      TCG_REG_S2,
>      TCG_REG_S3,
> @@ -1450,7 +1452,9 @@ static const TCGTargetOpDef mips_op_defs[] = {
>  };
>  
>  static int tcg_target_callee_save_regs[] = {
> +#if 0 /* used for the global env (TCG_AREG0), so no need to save */
>      TCG_REG_S0,
> +#endif
>      TCG_REG_S1,
>      TCG_REG_S2,
>      TCG_REG_S3,
> @@ -1459,8 +1463,7 @@ static int tcg_target_callee_save_regs[] = {
>      TCG_REG_S6,
>      TCG_REG_S7,
>      TCG_REG_GP,
> -    /* TCG_REG_FP, */ /* currently used for the global env, so np
> -                         need to save */
> +    TCG_REG_FP,
>      TCG_REG_RA,       /* should be last for ABI compliance */
>  };
>  
> diff --git a/tcg/mips/tcg-target.h b/tcg/mips/tcg-target.h
> index 0292d33..0028bfa 100644
> --- a/tcg/mips/tcg-target.h
> +++ b/tcg/mips/tcg-target.h
> @@ -97,7 +97,7 @@ enum {
>  #undef TCG_TARGET_HAS_ext16u_i32   /* andi rt, rs, 0xffff */
>  
>  /* Note: must be synced with dyngen-exec.h */
> -#define TCG_AREG0 TCG_REG_FP
> +#define TCG_AREG0 TCG_REG_S0
>  
>  /* guest base is supported */
>  #define TCG_TARGET_HAS_GUEST_BASE
> -- 
> 1.7.0
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net