Patchwork [05/10] tcg/mips: use stack for TCG temps

login
register
mail settings
Submitter Aurelien Jarno
Date Sept. 21, 2012, 4:43 p.m.
Message ID <1348245809-13482-6-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/185845/
State New
Headers show

Comments

Aurelien Jarno - Sept. 21, 2012, 4:43 p.m.
Use stack instead of temp_buf array in CPUState for TCG
temps.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 tcg/mips/tcg-target.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
Blue Swirl - Sept. 22, 2012, 2:37 p.m.
On Fri, Sep 21, 2012 at 4:43 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> Use stack instead of temp_buf array in CPUState for TCG
> temps.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  tcg/mips/tcg-target.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c
> index 0ea6a76..c05169f 100644
> --- a/tcg/mips/tcg-target.c
> +++ b/tcg/mips/tcg-target.c
> @@ -1538,11 +1538,15 @@ static void tcg_target_qemu_prologue(TCGContext *s)
>  {
>      int i, frame_size;
>
> -    /* reserve some stack space */
> +    /* reserve some stack space, also for TCG temps. */
>      frame_size = ARRAY_SIZE(tcg_target_callee_save_regs) * 4
> -                 + TCG_STATIC_CALL_ARGS_SIZE;
> +                 + TCG_STATIC_CALL_ARGS_SIZE
> +                 + CPU_TEMP_BUF_NLONGS * sizeof(long);
>      frame_size = (frame_size + TCG_TARGET_STACK_ALIGN - 1) &
>                   ~(TCG_TARGET_STACK_ALIGN - 1);
> +    tcg_set_frame(s, TCG_REG_SP, ARRAY_SIZE(tcg_target_callee_save_regs) * 4
> +                  + TCG_STATIC_CALL_ARGS_SIZE,
> +                  CPU_TEMP_BUF_NLONGS * sizeof(long));

My version used frame_size instead of duplicating a part of the
calculations, wouldn't that take stack alignment also in
consideration?

http://lists.nongnu.org/archive/html/qemu-devel/2011-06/msg02566.html

>
>      /* TB prologue */
>      tcg_out_addi(s, TCG_REG_SP, -frame_size);
> @@ -1597,6 +1601,4 @@ static void tcg_target_init(TCGContext *s)
>      tcg_regset_set_reg(s->reserved_regs, TCG_REG_GP);   /* global pointer */
>
>      tcg_add_target_add_op_defs(mips_op_defs);
> -    tcg_set_frame(s, TCG_AREG0, offsetof(CPUArchState, temp_buf),
> -                  CPU_TEMP_BUF_NLONGS * sizeof(long));
>  }
> --
> 1.7.10.4
>
>
Aurelien Jarno - Sept. 22, 2012, 5:25 p.m.
On Sat, Sep 22, 2012 at 02:37:35PM +0000, Blue Swirl wrote:
> On Fri, Sep 21, 2012 at 4:43 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > Use stack instead of temp_buf array in CPUState for TCG
> > temps.
> >
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> >  tcg/mips/tcg-target.c |   10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c
> > index 0ea6a76..c05169f 100644
> > --- a/tcg/mips/tcg-target.c
> > +++ b/tcg/mips/tcg-target.c
> > @@ -1538,11 +1538,15 @@ static void tcg_target_qemu_prologue(TCGContext *s)
> >  {
> >      int i, frame_size;
> >
> > -    /* reserve some stack space */
> > +    /* reserve some stack space, also for TCG temps. */
> >      frame_size = ARRAY_SIZE(tcg_target_callee_save_regs) * 4
> > -                 + TCG_STATIC_CALL_ARGS_SIZE;
> > +                 + TCG_STATIC_CALL_ARGS_SIZE
> > +                 + CPU_TEMP_BUF_NLONGS * sizeof(long);
> >      frame_size = (frame_size + TCG_TARGET_STACK_ALIGN - 1) &
> >                   ~(TCG_TARGET_STACK_ALIGN - 1);
> > +    tcg_set_frame(s, TCG_REG_SP, ARRAY_SIZE(tcg_target_callee_save_regs) * 4
> > +                  + TCG_STATIC_CALL_ARGS_SIZE,
> > +                  CPU_TEMP_BUF_NLONGS * sizeof(long));
> 
> My version used frame_size instead of duplicating a part of the
> calculations, wouldn't that take stack alignment also in
> consideration?
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2011-06/msg02566.html

Oh haven't seen this patch before. Well the choice is basically between
adding one value and subtracting one, not sure what is the best.

About the alignment, do we require some specific alignment for the
temps? As long as they are aligned wrt size, I think it's fine.

> >
> >      /* TB prologue */
> >      tcg_out_addi(s, TCG_REG_SP, -frame_size);
> > @@ -1597,6 +1601,4 @@ static void tcg_target_init(TCGContext *s)
> >      tcg_regset_set_reg(s->reserved_regs, TCG_REG_GP);   /* global pointer */
> >
> >      tcg_add_target_add_op_defs(mips_op_defs);
> > -    tcg_set_frame(s, TCG_AREG0, offsetof(CPUArchState, temp_buf),
> > -                  CPU_TEMP_BUF_NLONGS * sizeof(long));
> >  }
> > --
> > 1.7.10.4
> >
> >
>
Blue Swirl - Sept. 22, 2012, 6:09 p.m.
On Sat, Sep 22, 2012 at 5:25 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Sat, Sep 22, 2012 at 02:37:35PM +0000, Blue Swirl wrote:
>> On Fri, Sep 21, 2012 at 4:43 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> > Use stack instead of temp_buf array in CPUState for TCG
>> > temps.
>> >
>> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>> > ---
>> >  tcg/mips/tcg-target.c |   10 ++++++----
>> >  1 file changed, 6 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c
>> > index 0ea6a76..c05169f 100644
>> > --- a/tcg/mips/tcg-target.c
>> > +++ b/tcg/mips/tcg-target.c
>> > @@ -1538,11 +1538,15 @@ static void tcg_target_qemu_prologue(TCGContext *s)
>> >  {
>> >      int i, frame_size;
>> >
>> > -    /* reserve some stack space */
>> > +    /* reserve some stack space, also for TCG temps. */
>> >      frame_size = ARRAY_SIZE(tcg_target_callee_save_regs) * 4
>> > -                 + TCG_STATIC_CALL_ARGS_SIZE;
>> > +                 + TCG_STATIC_CALL_ARGS_SIZE
>> > +                 + CPU_TEMP_BUF_NLONGS * sizeof(long);
>> >      frame_size = (frame_size + TCG_TARGET_STACK_ALIGN - 1) &
>> >                   ~(TCG_TARGET_STACK_ALIGN - 1);
>> > +    tcg_set_frame(s, TCG_REG_SP, ARRAY_SIZE(tcg_target_callee_save_regs) * 4
>> > +                  + TCG_STATIC_CALL_ARGS_SIZE,
>> > +                  CPU_TEMP_BUF_NLONGS * sizeof(long));
>>
>> My version used frame_size instead of duplicating a part of the
>> calculations, wouldn't that take stack alignment also in
>> consideration?
>>
>> http://lists.nongnu.org/archive/html/qemu-devel/2011-06/msg02566.html
>
> Oh haven't seen this patch before. Well the choice is basically between
> adding one value and subtracting one, not sure what is the best.
>
> About the alignment, do we require some specific alignment for the
> temps? As long as they are aligned wrt size, I think it's fine.

Well, since we align the frame, that could be a better reference.
Usually stack doesn't get unaligned but why is there then the
alignment calculation?

>
>> >
>> >      /* TB prologue */
>> >      tcg_out_addi(s, TCG_REG_SP, -frame_size);
>> > @@ -1597,6 +1601,4 @@ static void tcg_target_init(TCGContext *s)
>> >      tcg_regset_set_reg(s->reserved_regs, TCG_REG_GP);   /* global pointer */
>> >
>> >      tcg_add_target_add_op_defs(mips_op_defs);
>> > -    tcg_set_frame(s, TCG_AREG0, offsetof(CPUArchState, temp_buf),
>> > -                  CPU_TEMP_BUF_NLONGS * sizeof(long));
>> >  }
>> > --
>> > 1.7.10.4
>> >
>> >
>>
>
> --
> Aurelien Jarno                          GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net
Aurelien Jarno - Sept. 22, 2012, 6:17 p.m.
On Sat, Sep 22, 2012 at 06:09:13PM +0000, Blue Swirl wrote:
> On Sat, Sep 22, 2012 at 5:25 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Sat, Sep 22, 2012 at 02:37:35PM +0000, Blue Swirl wrote:
> >> On Fri, Sep 21, 2012 at 4:43 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> >> > Use stack instead of temp_buf array in CPUState for TCG
> >> > temps.
> >> >
> >> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> >> > ---
> >> >  tcg/mips/tcg-target.c |   10 ++++++----
> >> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c
> >> > index 0ea6a76..c05169f 100644
> >> > --- a/tcg/mips/tcg-target.c
> >> > +++ b/tcg/mips/tcg-target.c
> >> > @@ -1538,11 +1538,15 @@ static void tcg_target_qemu_prologue(TCGContext *s)
> >> >  {
> >> >      int i, frame_size;
> >> >
> >> > -    /* reserve some stack space */
> >> > +    /* reserve some stack space, also for TCG temps. */
> >> >      frame_size = ARRAY_SIZE(tcg_target_callee_save_regs) * 4
> >> > -                 + TCG_STATIC_CALL_ARGS_SIZE;
> >> > +                 + TCG_STATIC_CALL_ARGS_SIZE
> >> > +                 + CPU_TEMP_BUF_NLONGS * sizeof(long);
> >> >      frame_size = (frame_size + TCG_TARGET_STACK_ALIGN - 1) &
> >> >                   ~(TCG_TARGET_STACK_ALIGN - 1);
> >> > +    tcg_set_frame(s, TCG_REG_SP, ARRAY_SIZE(tcg_target_callee_save_regs) * 4
> >> > +                  + TCG_STATIC_CALL_ARGS_SIZE,
> >> > +                  CPU_TEMP_BUF_NLONGS * sizeof(long));
> >>
> >> My version used frame_size instead of duplicating a part of the
> >> calculations, wouldn't that take stack alignment also in
> >> consideration?
> >>
> >> http://lists.nongnu.org/archive/html/qemu-devel/2011-06/msg02566.html
> >
> > Oh haven't seen this patch before. Well the choice is basically between
> > adding one value and subtracting one, not sure what is the best.
> >
> > About the alignment, do we require some specific alignment for the
> > temps? As long as they are aligned wrt size, I think it's fine.
> 
> Well, since we align the frame, that could be a better reference.
> Usually stack doesn't get unaligned but why is there then the
> alignment calculation?
> 

The ABI require the stack frame to be aligned to 8-bytes. It doesn't 
require any alignment for the data you put in the local parts.

Patch

diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c
index 0ea6a76..c05169f 100644
--- a/tcg/mips/tcg-target.c
+++ b/tcg/mips/tcg-target.c
@@ -1538,11 +1538,15 @@  static void tcg_target_qemu_prologue(TCGContext *s)
 {
     int i, frame_size;
 
-    /* reserve some stack space */
+    /* reserve some stack space, also for TCG temps. */
     frame_size = ARRAY_SIZE(tcg_target_callee_save_regs) * 4
-                 + TCG_STATIC_CALL_ARGS_SIZE;
+                 + TCG_STATIC_CALL_ARGS_SIZE
+                 + CPU_TEMP_BUF_NLONGS * sizeof(long);
     frame_size = (frame_size + TCG_TARGET_STACK_ALIGN - 1) &
                  ~(TCG_TARGET_STACK_ALIGN - 1);
+    tcg_set_frame(s, TCG_REG_SP, ARRAY_SIZE(tcg_target_callee_save_regs) * 4
+                  + TCG_STATIC_CALL_ARGS_SIZE,
+                  CPU_TEMP_BUF_NLONGS * sizeof(long));
 
     /* TB prologue */
     tcg_out_addi(s, TCG_REG_SP, -frame_size);
@@ -1597,6 +1601,4 @@  static void tcg_target_init(TCGContext *s)
     tcg_regset_set_reg(s->reserved_regs, TCG_REG_GP);   /* global pointer */
 
     tcg_add_target_add_op_defs(mips_op_defs);
-    tcg_set_frame(s, TCG_AREG0, offsetof(CPUArchState, temp_buf),
-                  CPU_TEMP_BUF_NLONGS * sizeof(long));
 }