diff mbox series

[V2] RISC-V: decouple stack allocation for rv32e w/o save-restore.

Message ID 20230429105959.23211-1-gaofei@eswincomputing.com
State Accepted
Headers
Series [V2] RISC-V: decouple stack allocation for rv32e w/o save-restore. |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Fei Gao April 29, 2023, 10:59 a.m. UTC 
  Currently in rv32e, stack allocation for GPR callee-saved registers is
always 12 bytes w/o save-restore. Actually, for the case without save-restore,
less stack memory can be reserved. This patch decouples stack allocation for
rv32e w/o save-restore and makes riscv_compute_frame_info more readable.

output of testcase rv32e_stack.c
before patch:
	addi	sp,sp,-16
	sw	ra,12(sp)
	call	getInt
	sw	a0,0(sp)
	lw	a0,0(sp)
	call	PrintInts
	lw	a5,0(sp)
	mv	a0,a5
	lw	ra,12(sp)
	addi	sp,sp,16
	jr	ra

after patch:
	addi	sp,sp,-8
	sw	ra,4(sp)
	call	getInt
	sw	a0,0(sp)
	lw	a0,0(sp)
	call	PrintInts
	lw	a5,0(sp)
	mv	a0,a5
	lw	ra,4(sp)
	addi	sp,sp,8
	jr	ra


gcc/ChangeLog:

        * config/riscv/riscv.cc (riscv_avoid_save_libcall): helper function for riscv_use_save_libcall.
        (riscv_use_save_libcall): call riscv_avoid_save_libcall.
        (riscv_compute_frame_info): restructure to decouple stack allocation for rv32e w/o save-restore.

gcc/testsuite/ChangeLog:

        * gcc.target/riscv/rv32e_stack.c: New test.
---
 gcc/config/riscv/riscv.cc                    | 58 ++++++++++++--------
 gcc/testsuite/gcc.target/riscv/rv32e_stack.c | 14 +++++
 2 files changed, 50 insertions(+), 22 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/rv32e_stack.c

Comments

Jeff Law April 29, 2023, 3:38 p.m. UTC | #1 
On 4/29/23 04:59, Fei Gao wrote:
> Currently in rv32e, stack allocation for GPR callee-saved registers is
> always 12 bytes w/o save-restore. Actually, for the case without save-restore,
> less stack memory can be reserved. This patch decouples stack allocation for
> rv32e w/o save-restore and makes riscv_compute_frame_info more readable.
> 
> output of testcase rv32e_stack.c
> before patch:
> 	addi	sp,sp,-16
> 	sw	ra,12(sp)
> 	call	getInt
> 	sw	a0,0(sp)
> 	lw	a0,0(sp)
> 	call	PrintInts
> 	lw	a5,0(sp)
> 	mv	a0,a5
> 	lw	ra,12(sp)
> 	addi	sp,sp,16
> 	jr	ra
> 
> after patch:
> 	addi	sp,sp,-8
> 	sw	ra,4(sp)
> 	call	getInt
> 	sw	a0,0(sp)
> 	lw	a0,0(sp)
> 	call	PrintInts
> 	lw	a5,0(sp)
> 	mv	a0,a5
> 	lw	ra,4(sp)
> 	addi	sp,sp,8
> 	jr	ra
> 
> 
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_avoid_save_libcall): helper function for riscv_use_save_libcall.
>          (riscv_use_save_libcall): call riscv_avoid_save_libcall.
>          (riscv_compute_frame_info): restructure to decouple stack allocation for rv32e w/o save-restore.
> 
> gcc/testsuite/ChangeLog:
> 
>          * gcc.target/riscv/rv32e_stack.c: New test.
Thanks.  I rewrapped the ChangeLog and pushed this to the trunk.

jeff
Palmer Dabbelt April 29, 2023, 5 p.m. UTC | #2 
On Sat, 29 Apr 2023 08:38:06 PDT (-0700), jeffreyalaw@gmail.com wrote:
>
>
> On 4/29/23 04:59, Fei Gao wrote:
>> Currently in rv32e, stack allocation for GPR callee-saved registers is
>> always 12 bytes w/o save-restore. Actually, for the case without save-restore,
>> less stack memory can be reserved. This patch decouples stack allocation for
>> rv32e w/o save-restore and makes riscv_compute_frame_info more readable.

Are you guys using rv32e?  It's not widely tested, at least by most 
upstream folks.  If you're actively trying to ship it then we should 
probably add it to the various lists of targest that get tested, as I'd 
bet there's a lot of oddness floating around.

>> output of testcase rv32e_stack.c
>> before patch:
>> 	addi	sp,sp,-16
>> 	sw	ra,12(sp)
>> 	call	getInt
>> 	sw	a0,0(sp)
>> 	lw	a0,0(sp)
>> 	call	PrintInts
>> 	lw	a5,0(sp)
>> 	mv	a0,a5
>> 	lw	ra,12(sp)
>> 	addi	sp,sp,16
>> 	jr	ra
>>
>> after patch:
>> 	addi	sp,sp,-8
>> 	sw	ra,4(sp)
>> 	call	getInt
>> 	sw	a0,0(sp)
>> 	lw	a0,0(sp)
>> 	call	PrintInts
>> 	lw	a5,0(sp)
>> 	mv	a0,a5
>> 	lw	ra,4(sp)
>> 	addi	sp,sp,8
>> 	jr	ra
>>
>>
>> gcc/ChangeLog:
>>
>>          * config/riscv/riscv.cc (riscv_avoid_save_libcall): helper function for riscv_use_save_libcall.
>>          (riscv_use_save_libcall): call riscv_avoid_save_libcall.
>>          (riscv_compute_frame_info): restructure to decouple stack allocation for rv32e w/o save-restore.
>>
>> gcc/testsuite/ChangeLog:
>>
>>          * gcc.target/riscv/rv32e_stack.c: New test.
> Thanks.  I rewrapped the ChangeLog and pushed this to the trunk.

Works for me, thanks for reviewing all this stuff -- we're all pretty 
buried ;)

>
> jeff
Jeff Law April 29, 2023, 5:44 p.m. UTC | #3 
On 4/29/23 11:00, Palmer Dabbelt wrote:
> On Sat, 29 Apr 2023 08:38:06 PDT (-0700), jeffreyalaw@gmail.com wrote:
>>
>>
>> On 4/29/23 04:59, Fei Gao wrote:
>>> Currently in rv32e, stack allocation for GPR callee-saved registers is
>>> always 12 bytes w/o save-restore. Actually, for the case without 
>>> save-restore,
>>> less stack memory can be reserved. This patch decouples stack 
>>> allocation for
>>> rv32e w/o save-restore and makes riscv_compute_frame_info more readable.
> 
> Are you guys using rv32e?  It's not widely tested, at least by most 
> upstream folks.  If you're actively trying to ship it then we should 
> probably add it to the various lists of targest that get tested, as I'd 
> bet there's a lot of oddness floating around.
No interest at all in rv32 at Ventana.


>> Thanks.  I rewrapped the ChangeLog and pushed this to the trunk.
> 
> Works for me, thanks for reviewing all this stuff -- we're all pretty 
> buried ;)
Just standard procedure with the trunk re-opened.  In the past I would 
have ignored anything in the risc-v space.  I've traded that for 
ignoring x86 :-)



Jeff
Palmer Dabbelt April 29, 2023, 5:45 p.m. UTC | #4 
On Sat, 29 Apr 2023 10:44:08 PDT (-0700), jeffreyalaw@gmail.com wrote:
>
>
> On 4/29/23 11:00, Palmer Dabbelt wrote:
>> On Sat, 29 Apr 2023 08:38:06 PDT (-0700), jeffreyalaw@gmail.com wrote:
>>>
>>>
>>> On 4/29/23 04:59, Fei Gao wrote:
>>>> Currently in rv32e, stack allocation for GPR callee-saved registers is
>>>> always 12 bytes w/o save-restore. Actually, for the case without
>>>> save-restore,
>>>> less stack memory can be reserved. This patch decouples stack
>>>> allocation for
>>>> rv32e w/o save-restore and makes riscv_compute_frame_info more readable.
>>
>> Are you guys using rv32e?  It's not widely tested, at least by most
>> upstream folks.  If you're actively trying to ship it then we should
>> probably add it to the various lists of targest that get tested, as I'd
>> bet there's a lot of oddness floating around.
> No interest at all in rv32 at Ventana.

Makes sense, I was mostly wondering abotu the Eswin folks though.

>
>
>>> Thanks.  I rewrapped the ChangeLog and pushed this to the trunk.
>>
>> Works for me, thanks for reviewing all this stuff -- we're all pretty
>> buried ;)
> Just standard procedure with the trunk re-opened.  In the past I would
> have ignored anything in the risc-v space.  I've traded that for
> ignoring x86 :-)
>
>
>
> Jeff
Kito Cheng April 30, 2023, 1:44 a.m. UTC | #5 
SiFive has tests and delivers RV32E.

On Sun, Apr 30, 2023 at 1:45 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Sat, 29 Apr 2023 10:44:08 PDT (-0700), jeffreyalaw@gmail.com wrote:
> >
> >
> > On 4/29/23 11:00, Palmer Dabbelt wrote:
> >> On Sat, 29 Apr 2023 08:38:06 PDT (-0700), jeffreyalaw@gmail.com wrote:
> >>>
> >>>
> >>> On 4/29/23 04:59, Fei Gao wrote:
> >>>> Currently in rv32e, stack allocation for GPR callee-saved registers is
> >>>> always 12 bytes w/o save-restore. Actually, for the case without
> >>>> save-restore,
> >>>> less stack memory can be reserved. This patch decouples stack
> >>>> allocation for
> >>>> rv32e w/o save-restore and makes riscv_compute_frame_info more readable.
> >>
> >> Are you guys using rv32e?  It's not widely tested, at least by most
> >> upstream folks.  If you're actively trying to ship it then we should
> >> probably add it to the various lists of targest that get tested, as I'd
> >> bet there's a lot of oddness floating around.
> > No interest at all in rv32 at Ventana.
>
> Makes sense, I was mostly wondering abotu the Eswin folks though.
>
> >
> >
> >>> Thanks.  I rewrapped the ChangeLog and pushed this to the trunk.
> >>
> >> Works for me, thanks for reviewing all this stuff -- we're all pretty
> >> buried ;)
> > Just standard procedure with the trunk re-opened.  In the past I would
> > have ignored anything in the risc-v space.  I've traded that for
> > ignoring x86 :-)
> >
> >
> >
> > Jeff
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 5d2550871c7..8b32977e296 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -4772,12 +4772,27 @@  riscv_save_reg_p (unsigned int regno)
   return false;
 }
 
+/* Return TRUE if a libcall to save/restore GPRs should be
+   avoided.  FALSE otherwise.  */
+static bool
+riscv_avoid_save_libcall (void)
+{
+  if (!TARGET_SAVE_RESTORE
+      || crtl->calls_eh_return
+      || frame_pointer_needed
+      || cfun->machine->interrupt_handler_p
+      || cfun->machine->varargs_size != 0
+      || crtl->args.pretend_args_size != 0)
+    return true;
+
+  return false;
+}
+
 /* Determine whether to call GPR save/restore routines.  */
 static bool
 riscv_use_save_libcall (const struct riscv_frame_info *frame)
 {
-  if (!TARGET_SAVE_RESTORE || crtl->calls_eh_return || frame_pointer_needed
-      || cfun->machine->interrupt_handler_p)
+  if (riscv_avoid_save_libcall ())
     return false;
 
   return frame->save_libcall_adjustment != 0;
@@ -4857,7 +4872,7 @@  riscv_compute_frame_info (void)
   struct riscv_frame_info *frame;
   poly_int64 offset;
   bool interrupt_save_prologue_temp = false;
-  unsigned int regno, i, num_x_saved = 0, num_f_saved = 0;
+  unsigned int regno, i, num_x_saved = 0, num_f_saved = 0, x_save_size = 0;
 
   frame = &cfun->machine->frame;
 
@@ -4895,24 +4910,14 @@  riscv_compute_frame_info (void)
 	    frame->fmask |= 1 << (regno - FP_REG_FIRST), num_f_saved++;
     }
 
-  /* At the bottom of the frame are any outgoing stack arguments. */
-  offset = riscv_stack_align (crtl->outgoing_args_size);
-  /* Next are local stack variables. */
-  offset += riscv_stack_align (get_frame_size ());
-  /* The virtual frame pointer points above the local variables. */
-  frame->frame_pointer_offset = offset;
-  /* Next are the callee-saved FPRs. */
-  if (frame->fmask)
-    offset += riscv_stack_align (num_f_saved * UNITS_PER_FP_REG);
-  frame->fp_sp_offset = offset - UNITS_PER_FP_REG;
-  /* Next are the callee-saved GPRs. */
   if (frame->mask)
     {
-      unsigned x_save_size = riscv_stack_align (num_x_saved * UNITS_PER_WORD);
+      x_save_size = riscv_stack_align (num_x_saved * UNITS_PER_WORD);
       unsigned num_save_restore = 1 + riscv_save_libcall_count (frame->mask);
 
       /* Only use save/restore routines if they don't alter the stack size.  */
-      if (riscv_stack_align (num_save_restore * UNITS_PER_WORD) == x_save_size)
+      if (riscv_stack_align (num_save_restore * UNITS_PER_WORD) == x_save_size
+          && !riscv_avoid_save_libcall ())
 	{
 	  /* Libcall saves/restores 3 registers at once, so we need to
 	     allocate 12 bytes for callee-saved register.  */
@@ -4921,9 +4926,21 @@  riscv_compute_frame_info (void)
 
 	  frame->save_libcall_adjustment = x_save_size;
 	}
-
-      offset += x_save_size;
     }
+
+  /* At the bottom of the frame are any outgoing stack arguments. */
+  offset = riscv_stack_align (crtl->outgoing_args_size);
+  /* Next are local stack variables. */
+  offset += riscv_stack_align (get_frame_size ());
+  /* The virtual frame pointer points above the local variables. */
+  frame->frame_pointer_offset = offset;
+  /* Next are the callee-saved FPRs. */
+  if (frame->fmask)
+    offset += riscv_stack_align (num_f_saved * UNITS_PER_FP_REG);
+  frame->fp_sp_offset = offset - UNITS_PER_FP_REG;
+  /* Next are the callee-saved GPRs. */
+  if (frame->mask)
+    offset += x_save_size;
   frame->gp_sp_offset = offset - UNITS_PER_WORD;
   /* The hard frame pointer points above the callee-saved GPRs. */
   frame->hard_frame_pointer_offset = offset;
@@ -4935,11 +4952,8 @@  riscv_compute_frame_info (void)
      padding.  */
   frame->arg_pointer_offset = offset - crtl->args.pretend_args_size;
   frame->total_size = offset;
-  /* Next points the incoming stack pointer and any incoming arguments. */
 
-  /* Only use save/restore routines when the GPRs are atop the frame.  */
-  if (known_ne (frame->hard_frame_pointer_offset, frame->total_size))
-    frame->save_libcall_adjustment = 0;
+  /* Next points the incoming stack pointer and any incoming arguments. */
 }
 
 /* Make sure that we're not trying to eliminate to the wrong hard frame
diff --git a/gcc/testsuite/gcc.target/riscv/rv32e_stack.c b/gcc/testsuite/gcc.target/riscv/rv32e_stack.c
new file mode 100644
index 00000000000..cec90ede4b5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rv32e_stack.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile} */
+/* { dg-options "-O0 -march=rv32e -mabi=ilp32e -fomit-frame-pointer" } */
+
+int getInt();
+void PrintInts(int);
+
+int callPrintInts()
+{
+  int i = getInt();
+  PrintInts(i);
+  return i;
+}
+
+/* { dg-final { scan-assembler-not "addi	sp,sp,-16" } } */