[PATCH 0/3] RISC-V: optimize stack manipulation in save-restore

[PATCH 0/3] RISC-V: optimize stack manipulation in save-restore

[Fei Gao]

The patches allow less instructions to be used in stack allocation 
and deallocation if save-restore enabled, and also make the stack 
manipulation codes more readable.

Fei Gao (3):
  RISC-V: add a new parameter in riscv_first_stack_step.
  RISC-V: optimize stack manipulation in save-restore
  RISC-V: make the stack manipulation codes more readable.

 gcc/config/riscv/riscv.cc                     | 105 +++++++++---------
 .../gcc.target/riscv/stack_save_restore.c     |  40 +++++++
 2 files changed, 95 insertions(+), 50 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/stack_save_restore.c

-- 
2.17.1

[PATCH 1/3] RISC-V: add a new parameter in riscv_first_stack_step.

[Fei Gao]

frame->total_size to remaining_size conversion is done as an independent patch without
functionality change as per review comment.

gcc/ChangeLog:

        * config/riscv/riscv.cc (riscv_first_stack_step): add a new function parameter remaining_size.
        (riscv_compute_frame_info): adapt new riscv_first_stack_step interface.
        (riscv_expand_prologue): likewise.
        (riscv_expand_epilogue): likewise.
---
 gcc/config/riscv/riscv.cc | 48 +++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 05bdba5ab4d..f0bbcd6d6be 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -4634,7 +4634,7 @@ riscv_save_libcall_count (unsigned mask)
    They decrease stack_pointer_rtx but leave frame_pointer_rtx and
    hard_frame_pointer_rtx unchanged.  */
 
-static HOST_WIDE_INT riscv_first_stack_step (struct riscv_frame_info *frame);
+static HOST_WIDE_INT riscv_first_stack_step (struct riscv_frame_info *frame, poly_int64 remaining_size);
 
 /* Handle stack align for poly_int.  */
 static poly_int64
@@ -4663,7 +4663,7 @@ riscv_compute_frame_info (void)
      save/restore t0.  We check for this before clearing the frame struct.  */
   if (cfun->machine->interrupt_handler_p)
     {
-      HOST_WIDE_INT step1 = riscv_first_stack_step (frame);
+      HOST_WIDE_INT step1 = riscv_first_stack_step (frame, frame->total_size);
       if (! POLY_SMALL_OPERAND_P ((frame->total_size - step1)))
 	interrupt_save_prologue_temp = true;
     }
@@ -4913,45 +4913,45 @@ riscv_restore_reg (rtx reg, rtx mem)
    without adding extra instructions.  */
 
 static HOST_WIDE_INT
-riscv_first_stack_step (struct riscv_frame_info *frame)
+riscv_first_stack_step (struct riscv_frame_info *frame, poly_int64 remaining_size)
 {
-  HOST_WIDE_INT frame_total_constant_size;
-  if (!frame->total_size.is_constant ())
-    frame_total_constant_size
-      = riscv_stack_align (frame->total_size.coeffs[0])
-	- riscv_stack_align (frame->total_size.coeffs[1]);
+  HOST_WIDE_INT remaining_const_size;
+  if (!remaining_size.is_constant ())
+    remaining_const_size
+      = riscv_stack_align (remaining_size.coeffs[0])
+        - riscv_stack_align (remaining_size.coeffs[1]);
   else
-    frame_total_constant_size = frame->total_size.to_constant ();
+    remaining_const_size = remaining_size.to_constant ();
 
-  if (SMALL_OPERAND (frame_total_constant_size))
-    return frame_total_constant_size;
+  if (SMALL_OPERAND (remaining_const_size))
+    return remaining_const_size;
 
   HOST_WIDE_INT min_first_step =
-    RISCV_STACK_ALIGN ((frame->total_size - frame->frame_pointer_offset).to_constant());
+    riscv_stack_align ((remaining_size - frame->frame_pointer_offset).to_constant());
   HOST_WIDE_INT max_first_step = IMM_REACH / 2 - PREFERRED_STACK_BOUNDARY / 8;
-  HOST_WIDE_INT min_second_step = frame_total_constant_size - max_first_step;
+  HOST_WIDE_INT min_second_step = remaining_const_size - max_first_step;
   gcc_assert (min_first_step <= max_first_step);
 
   /* As an optimization, use the least-significant bits of the total frame
      size, so that the second adjustment step is just LUI + ADD.  */
   if (!SMALL_OPERAND (min_second_step)
-      && frame_total_constant_size % IMM_REACH < IMM_REACH / 2
-      && frame_total_constant_size % IMM_REACH >= min_first_step)
-    return frame_total_constant_size % IMM_REACH;
+      && remaining_const_size % IMM_REACH < IMM_REACH / 2
+      && remaining_const_size % IMM_REACH >= min_first_step)
+    return remaining_const_size % IMM_REACH;
 
   if (TARGET_RVC)
     {
       /* If we need two subtracts, and one is small enough to allow compressed
-	 loads and stores, then put that one first.  */
+         loads and stores, then put that one first.  */
       if (IN_RANGE (min_second_step, 0,
-		    (TARGET_64BIT ? SDSP_REACH : SWSP_REACH)))
-	return MAX (min_second_step, min_first_step);
+                    (TARGET_64BIT ? SDSP_REACH : SWSP_REACH)))
+       return MAX (min_second_step, min_first_step);
 
       /* If we need LUI + ADDI + ADD for the second adjustment step, then start
-	 with the minimum first step, so that we can get compressed loads and
-	 stores.  */
+         with the minimum first step, so that we can get compressed loads and
+         stores.  */
       else if (!SMALL_OPERAND (min_second_step))
-	return min_first_step;
+       return min_first_step;
     }
 
   return max_first_step;
@@ -5037,7 +5037,7 @@ riscv_expand_prologue (void)
   /* Save the registers.  */
   if ((frame->mask | frame->fmask) != 0)
     {
-      HOST_WIDE_INT step1 = riscv_first_stack_step (frame);
+      HOST_WIDE_INT step1 = riscv_first_stack_step (frame, frame->total_size);
       if (size.is_constant ())
 	step1 = MIN (size.to_constant(), step1);
 
@@ -5216,7 +5216,7 @@ riscv_expand_epilogue (int style)
      possible in the second step without going out of range.  */
   if ((frame->mask | frame->fmask) != 0)
     {
-      step2 = riscv_first_stack_step (frame);
+      step2 = riscv_first_stack_step (frame, frame->total_size);
       step1 -= step2;
     }
 
-- 
2.17.1

[PING][PATCH 1/3] RISC-V: add a new parameter in riscv_first_stack_step.

[Fei Gao]

Gentle ping.

The patch I previously submitted:
| Date: Wed, 30 Nov 2022 00:38:08 -0800
| Subject: [PATCH] RISC-V: optimize stack manipulation in save-restore
| Message-ID: <gaofei@eswincomputing.com>

I split the patches as per Palmer's review comment.

BR
Fei

>frame->total_size to remaining_size conversion is done as an independent patch without
>functionality change as per review comment.
>
>gcc/ChangeLog:
>
>        * config/riscv/riscv.cc (riscv_first_stack_step): add a new function parameter remaining_size.
>        (riscv_compute_frame_info): adapt new riscv_first_stack_step interface.
>        (riscv_expand_prologue): likewise.
>        (riscv_expand_epilogue): likewise.
>---
> gcc/config/riscv/riscv.cc | 48 +++++++++++++++++++--------------------
> 1 file changed, 24 insertions(+), 24 deletions(-)
>
>diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>index 05bdba5ab4d..f0bbcd6d6be 100644
>--- a/gcc/config/riscv/riscv.cc
>+++ b/gcc/config/riscv/riscv.cc
>@@ -4634,7 +4634,7 @@ riscv_save_libcall_count (unsigned mask)
>    They decrease stack_pointer_rtx but leave frame_pointer_rtx and
>    hard_frame_pointer_rtx unchanged.  */
>
>-static HOST_WIDE_INT riscv_first_stack_step (struct riscv_frame_info *frame);
>+static HOST_WIDE_INT riscv_first_stack_step (struct riscv_frame_info *frame, poly_int64 remaining_size);
>
> /* Handle stack align for poly_int.  */
> static poly_int64
>@@ -4663,7 +4663,7 @@ riscv_compute_frame_info (void)
>      save/restore t0.  We check for this before clearing the frame struct.  */
>   if (cfun->machine->interrupt_handler_p)
>     {
>-      HOST_WIDE_INT step1 = riscv_first_stack_step (frame);
>+      HOST_WIDE_INT step1 = riscv_first_stack_step (frame, frame->total_size);
>       if (! POLY_SMALL_OPERAND_P ((frame->total_size - step1)))
> interrupt_save_prologue_temp = true;
>     }
>@@ -4913,45 +4913,45 @@ riscv_restore_reg (rtx reg, rtx mem)
>    without adding extra instructions.  */
>
> static HOST_WIDE_INT
>-riscv_first_stack_step (struct riscv_frame_info *frame)
>+riscv_first_stack_step (struct riscv_frame_info *frame, poly_int64 remaining_size)
> {
>-  HOST_WIDE_INT frame_total_constant_size;
>-  if (!frame->total_size.is_constant ())
>-    frame_total_constant_size
>-      = riscv_stack_align (frame->total_size.coeffs[0])
>-	- riscv_stack_align (frame->total_size.coeffs[1]);
>+  HOST_WIDE_INT remaining_const_size;
>+  if (!remaining_size.is_constant ())
>+    remaining_const_size
>+      = riscv_stack_align (remaining_size.coeffs[0])
>+        - riscv_stack_align (remaining_size.coeffs[1]);
>   else
>-    frame_total_constant_size = frame->total_size.to_constant ();
>+    remaining_const_size = remaining_size.to_constant ();
>
>-  if (SMALL_OPERAND (frame_total_constant_size))
>-    return frame_total_constant_size;
>+  if (SMALL_OPERAND (remaining_const_size))
>+    return remaining_const_size;
>
>   HOST_WIDE_INT min_first_step =
>-    RISCV_STACK_ALIGN ((frame->total_size - frame->frame_pointer_offset).to_constant());
>+    riscv_stack_align ((remaining_size - frame->frame_pointer_offset).to_constant());
>   HOST_WIDE_INT max_first_step = IMM_REACH / 2 - PREFERRED_STACK_BOUNDARY / 8;
>-  HOST_WIDE_INT min_second_step = frame_total_constant_size - max_first_step;
>+  HOST_WIDE_INT min_second_step = remaining_const_size - max_first_step;
>   gcc_assert (min_first_step <= max_first_step);
>
>   /* As an optimization, use the least-significant bits of the total frame
>      size, so that the second adjustment step is just LUI + ADD.  */
>   if (!SMALL_OPERAND (min_second_step)
>-      && frame_total_constant_size % IMM_REACH < IMM_REACH / 2
>-      && frame_total_constant_size % IMM_REACH >= min_first_step)
>-    return frame_total_constant_size % IMM_REACH;
>+      && remaining_const_size % IMM_REACH < IMM_REACH / 2
>+      && remaining_const_size % IMM_REACH >= min_first_step)
>+    return remaining_const_size % IMM_REACH;
>
>   if (TARGET_RVC)
>     {
>       /* If we need two subtracts, and one is small enough to allow compressed
>-	loads and stores, then put that one first.  */
>+         loads and stores, then put that one first.  */
>       if (IN_RANGE (min_second_step, 0,
>-	    (TARGET_64BIT ? SDSP_REACH : SWSP_REACH)))
>-	return MAX (min_second_step, min_first_step);
>+                    (TARGET_64BIT ? SDSP_REACH : SWSP_REACH)))
>+       return MAX (min_second_step, min_first_step);
>
>       /* If we need LUI + ADDI + ADD for the second adjustment step, then start
>-	with the minimum first step, so that we can get compressed loads and
>-	stores.  */
>+         with the minimum first step, so that we can get compressed loads and
>+         stores.  */
>       else if (!SMALL_OPERAND (min_second_step))
>-	return min_first_step;
>+       return min_first_step;
>     }
>
>   return max_first_step;
>@@ -5037,7 +5037,7 @@ riscv_expand_prologue (void)
>   /* Save the registers.  */
>   if ((frame->mask | frame->fmask) != 0)
>     {
>-      HOST_WIDE_INT step1 = riscv_first_stack_step (frame);
>+      HOST_WIDE_INT step1 = riscv_first_stack_step (frame, frame->total_size);
>       if (size.is_constant ())
> step1 = MIN (size.to_constant(), step1);
>
>@@ -5216,7 +5216,7 @@ riscv_expand_epilogue (int style)
>      possible in the second step without going out of range.  */
>   if ((frame->mask | frame->fmask) != 0)
>     {
>-      step2 = riscv_first_stack_step (frame);
>+      step2 = riscv_first_stack_step (frame, frame->total_size);
>       step1 -= step2;
>     }
>
>--
>2.17.1

Re: [PATCH 1/3] RISC-V: add a new parameter in riscv_first_stack_step.

[Jeff Law]

On 12/1/22 03:03, Fei Gao wrote:
> frame->total_size to remaining_size conversion is done as an independent patch without
> functionality change as per review comment.
> 
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_first_stack_step): add a new function parameter remaining_size.
>          (riscv_compute_frame_info): adapt new riscv_first_stack_step interface.
>          (riscv_expand_prologue): likewise.
>          (riscv_expand_epilogue): likewise.
> ---
>   gcc/config/riscv/riscv.cc | 48 +++++++++++++++++++--------------------
>   1 file changed, 24 insertions(+), 24 deletions(-)
> 




> @@ -5037,7 +5037,7 @@ riscv_expand_prologue (void)
>     /* Save the registers.  */
>     if ((frame->mask | frame->fmask) != 0)
>       {
> -      HOST_WIDE_INT step1 = riscv_first_stack_step (frame);
> +      HOST_WIDE_INT step1 = riscv_first_stack_step (frame, frame->total_size);
>         if (size.is_constant ())
>   	step1 = MIN (size.to_constant(), step1);
Hmm.  I generally agree that this patch has no functional changes in 
behavior.   But I wonder if there's a latent bug in the prologue code.

It seems to me that if we are optimizing for size and need to save both 
GPRs and FPRs that we don't want to be using frame->total_size as the 
libcall path to save the GPRs will have done a partial allocation, thus 
reducing the amount of stack still to allocate.  Or am I missing 
something here?



Jeff

Re: [PATCH 1/3] RISC-V: add a new parameter in riscv_first_stack_step.

[Jeff Law]

On 12/1/22 03:03, Fei Gao wrote:
> frame->total_size to remaining_size conversion is done as an independent patch without
> functionality change as per review comment.
> 
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_first_stack_step): add a new function parameter remaining_size.
>          (riscv_compute_frame_info): adapt new riscv_first_stack_step interface.
>          (riscv_expand_prologue): likewise.
>          (riscv_expand_epilogue): likewise.
I fixed the capitalization issues in the ChangeLog and a few places 
where tabs should have been used instead of spaces and pushed this to 
the trunk.

Thanks for your patience,

Jeff

[PATCH 2/3] RISC-V: optimize stack manipulation in save-restore

[Fei Gao]

The stack that save-restore reserves is not well accumulated in stack allocation and deallocation.
This patch allows less instructions to be used in stack allocation and deallocation if save-restore enabled.

before patch:
  bar:
    call	t0,__riscv_save_4
    addi	sp,sp,-64
    ...
    li	t0,-12288
    addi	t0,t0,-1968 # optimized out after patch
    add	sp,sp,t0 # prologue
    ...
    li	t0,12288 # epilogue
    addi	t0,t0,2000 # optimized out after patch
    add	sp,sp,t0
    ...
    addi	sp,sp,32
    tail	__riscv_restore_4

after patch:
  bar:
    call	t0,__riscv_save_4
    addi	sp,sp,-2032
    ...
    li	t0,-12288
    add	sp,sp,t0 # prologue
    ...
    li	t0,12288 # epilogue
    add	sp,sp,t0
    ...
    addi	sp,sp,2032
    tail	__riscv_restore_4

gcc/ChangeLog:

        * config/riscv/riscv.cc (riscv_expand_prologue): consider save-restore in stack allocation.
        (riscv_expand_epilogue): consider save-restore in stack deallocation.

gcc/testsuite/ChangeLog:

        * gcc.target/riscv/stack_save_restore.c: New test.
---
 gcc/config/riscv/riscv.cc                     | 50 ++++++++++---------
 .../gcc.target/riscv/stack_save_restore.c     | 40 +++++++++++++++
 2 files changed, 66 insertions(+), 24 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/stack_save_restore.c

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index f0bbcd6d6be..a50f2303032 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -5010,12 +5010,12 @@ void
 riscv_expand_prologue (void)
 {
   struct riscv_frame_info *frame = &cfun->machine->frame;
-  poly_int64 size = frame->total_size;
+  poly_int64 remaining_size = frame->total_size;
   unsigned mask = frame->mask;
   rtx insn;
 
   if (flag_stack_usage_info)
-    current_function_static_stack_size = constant_lower_bound (size);
+    current_function_static_stack_size = constant_lower_bound (remaining_size);
 
   if (cfun->machine->naked_p)
     return;
@@ -5026,7 +5026,7 @@ riscv_expand_prologue (void)
       rtx dwarf = NULL_RTX;
       dwarf = riscv_adjust_libcall_cfi_prologue ();
 
-      size -= frame->save_libcall_adjustment;
+      remaining_size -= frame->save_libcall_adjustment;
       insn = emit_insn (riscv_gen_gpr_save_insn (frame));
       frame->mask = 0; /* Temporarily fib that we need not save GPRs.  */
 
@@ -5037,16 +5037,14 @@ riscv_expand_prologue (void)
   /* Save the registers.  */
   if ((frame->mask | frame->fmask) != 0)
     {
-      HOST_WIDE_INT step1 = riscv_first_stack_step (frame, frame->total_size);
-      if (size.is_constant ())
-	step1 = MIN (size.to_constant(), step1);
+      HOST_WIDE_INT step1 = riscv_first_stack_step (frame, remaining_size);
 
       insn = gen_add3_insn (stack_pointer_rtx,
 			    stack_pointer_rtx,
 			    GEN_INT (-step1));
       RTX_FRAME_RELATED_P (emit_insn (insn)) = 1;
-      size -= step1;
-      riscv_for_each_saved_reg (size, riscv_save_reg, false, false);
+      remaining_size -= step1;
+      riscv_for_each_saved_reg (remaining_size, riscv_save_reg, false, false);
     }
 
   frame->mask = mask; /* Undo the above fib.  */
@@ -5055,29 +5053,29 @@ riscv_expand_prologue (void)
   if (frame_pointer_needed)
     {
       insn = gen_add3_insn (hard_frame_pointer_rtx, stack_pointer_rtx,
-			    GEN_INT ((frame->hard_frame_pointer_offset - size).to_constant ()));
+			    GEN_INT ((frame->hard_frame_pointer_offset - remaining_size).to_constant ()));
       RTX_FRAME_RELATED_P (emit_insn (insn)) = 1;
 
       riscv_emit_stack_tie ();
     }
 
   /* Allocate the rest of the frame.  */
-  if (known_gt (size, 0))
+  if (known_gt (remaining_size, 0))
     {
       /* Two step adjustment:
 	 1.scalable frame. 2.constant frame.  */
       poly_int64 scalable_frame (0, 0);
-      if (!size.is_constant ())
+      if (!remaining_size.is_constant ())
 	{
 	  /* First for scalable frame.  */
-	  poly_int64 scalable_frame = size;
-	  scalable_frame.coeffs[0] = size.coeffs[1];
+	  poly_int64 scalable_frame = remaining_size;
+	  scalable_frame.coeffs[0] = remaining_size.coeffs[1];
 	  riscv_v_adjust_scalable_frame (stack_pointer_rtx, scalable_frame, false);
-	  size -= scalable_frame;
+	  remaining_size -= scalable_frame;
 	}
 
       /* Second step for constant frame.  */
-      HOST_WIDE_INT constant_frame = size.to_constant ();
+      HOST_WIDE_INT constant_frame = remaining_size.to_constant ();
       if (constant_frame == 0)
 	return;
 
@@ -5142,6 +5140,8 @@ riscv_expand_epilogue (int style)
   HOST_WIDE_INT step2 = 0;
   bool use_restore_libcall = ((style == NORMAL_RETURN)
 			      && riscv_use_save_libcall (frame));
+  unsigned libcall_size = use_restore_libcall ?
+                            frame->save_libcall_adjustment : 0;
   rtx ra = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
   rtx insn;
 
@@ -5212,13 +5212,18 @@ riscv_expand_epilogue (int style)
       REG_NOTES (insn) = dwarf;
     }
 
+  if (use_restore_libcall)
+    frame->mask = 0; /* Temporarily fib for GPRs.  */
+
   /* If we need to restore registers, deallocate as much stack as
      possible in the second step without going out of range.  */
   if ((frame->mask | frame->fmask) != 0)
-    {
-      step2 = riscv_first_stack_step (frame, frame->total_size);
-      step1 -= step2;
-    }
+    step2 = riscv_first_stack_step (frame, frame->total_size - libcall_size);
+
+  if (use_restore_libcall)
+    frame->mask = mask; /* Undo the above fib.  */
+
+  step1 -= step2 + libcall_size;
 
   /* Set TARGET to BASE + STEP1.  */
   if (known_gt (step1, 0))
@@ -5272,15 +5277,12 @@ riscv_expand_epilogue (int style)
     frame->mask = 0; /* Temporarily fib that we need not save GPRs.  */
 
   /* Restore the registers.  */
-  riscv_for_each_saved_reg (frame->total_size - step2, riscv_restore_reg,
+  riscv_for_each_saved_reg (frame->total_size - step2 - libcall_size,
+                            riscv_restore_reg,
 			    true, style == EXCEPTION_RETURN);
 
   if (use_restore_libcall)
-    {
       frame->mask = mask; /* Undo the above fib.  */
-      gcc_assert (step2 >= frame->save_libcall_adjustment);
-      step2 -= frame->save_libcall_adjustment;
-    }
 
   if (need_barrier_p)
     riscv_emit_stack_tie ();
diff --git a/gcc/testsuite/gcc.target/riscv/stack_save_restore.c b/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
new file mode 100644
index 00000000000..522e706cfbf
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
@@ -0,0 +1,40 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32imafc -mabi=ilp32f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto" } */
+/* { dg-final { check-function-bodies "**" "" } } */
+
+char my_getchar();
+float getf();
+
+/*
+**bar:
+**	call	t0,__riscv_save_4
+**	addi	sp,sp,-2032
+**	...
+**	li	t0,-12288
+**	add	sp,sp,t0
+**	...
+**	li	t0,12288
+**	add	sp,sp,t0
+**	...
+**	addi	sp,sp,2032
+**	tail	__riscv_restore_4
+*/
+int bar()
+{
+  float volatile farray[3568];
+
+  float sum = 0;
+  float f1 = getf();
+  float f2 = getf();
+  float f3 = getf();
+  float f4 = getf();
+
+  for (int i = 0; i < 3568; i++)
+  {
+    farray[i] = my_getchar() * 1.2;
+    sum += farray[i];
+  }
+
+  return sum + f1 + f2 + f3 + f4;
+}
+
-- 
2.17.1

[PING] [PATCH 2/3] RISC-V: optimize stack manipulation in save-restore

[Fei Gao]

Gentle ping.

The patch I previously submitted:
| Date: Wed, 30 Nov 2022 00:38:08 -0800
| Subject: [PATCH] RISC-V: optimize stack manipulation in save-restore
| Message-ID: <gaofei@eswincomputing.com>

I split the patches as per Palmer's review comment.

BR
Fei

>The stack that save-restore reserves is not well accumulated in stack allocation and deallocation.
>This patch allows less instructions to be used in stack allocation and deallocation if save-restore enabled.
>
>before patch:
>  bar:
>    call	t0,__riscv_save_4
>    addi	sp,sp,-64
>    ...
>    li	t0,-12288
>    addi	t0,t0,-1968 # optimized out after patch
>    add	sp,sp,t0 # prologue
>    ...
>    li	t0,12288 # epilogue
>    addi	t0,t0,2000 # optimized out after patch
>    add	sp,sp,t0
>    ...
>    addi	sp,sp,32
>    tail	__riscv_restore_4
>
>after patch:
>  bar:
>    call	t0,__riscv_save_4
>    addi	sp,sp,-2032
>    ...
>    li	t0,-12288
>    add	sp,sp,t0 # prologue
>    ...
>    li	t0,12288 # epilogue
>    add	sp,sp,t0
>    ...
>    addi	sp,sp,2032
>    tail	__riscv_restore_4
>
>gcc/ChangeLog:
>
>        * config/riscv/riscv.cc (riscv_expand_prologue): consider save-restore in stack allocation.
>        (riscv_expand_epilogue): consider save-restore in stack deallocation.
>
>gcc/testsuite/ChangeLog:
>
>        * gcc.target/riscv/stack_save_restore.c: New test.
>---
> gcc/config/riscv/riscv.cc                     | 50 ++++++++++---------
> .../gcc.target/riscv/stack_save_restore.c     | 40 +++++++++++++++
> 2 files changed, 66 insertions(+), 24 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/stack_save_restore.c
>
>diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>index f0bbcd6d6be..a50f2303032 100644
>--- a/gcc/config/riscv/riscv.cc
>+++ b/gcc/config/riscv/riscv.cc
>@@ -5010,12 +5010,12 @@ void
> riscv_expand_prologue (void)
> {
>   struct riscv_frame_info *frame = &cfun->machine->frame;
>-  poly_int64 size = frame->total_size;
>+  poly_int64 remaining_size = frame->total_size;
>   unsigned mask = frame->mask;
>   rtx insn;
>
>   if (flag_stack_usage_info)
>-    current_function_static_stack_size = constant_lower_bound (size);
>+    current_function_static_stack_size = constant_lower_bound (remaining_size);
>
>   if (cfun->machine->naked_p)
>     return;
>@@ -5026,7 +5026,7 @@ riscv_expand_prologue (void)
>       rtx dwarf = NULL_RTX;
>       dwarf = riscv_adjust_libcall_cfi_prologue ();
>
>-      size -= frame->save_libcall_adjustment;
>+      remaining_size -= frame->save_libcall_adjustment;
>       insn = emit_insn (riscv_gen_gpr_save_insn (frame));
>       frame->mask = 0; /* Temporarily fib that we need not save GPRs.  */
>
>@@ -5037,16 +5037,14 @@ riscv_expand_prologue (void)
>   /* Save the registers.  */
>   if ((frame->mask | frame->fmask) != 0)
>     {
>-      HOST_WIDE_INT step1 = riscv_first_stack_step (frame, frame->total_size);
>-      if (size.is_constant ())
>-	step1 = MIN (size.to_constant(), step1);
>+      HOST_WIDE_INT step1 = riscv_first_stack_step (frame, remaining_size);
>
>       insn = gen_add3_insn (stack_pointer_rtx,
>     stack_pointer_rtx,
>     GEN_INT (-step1));
>       RTX_FRAME_RELATED_P (emit_insn (insn)) = 1;
>-      size -= step1;
>-      riscv_for_each_saved_reg (size, riscv_save_reg, false, false);
>+      remaining_size -= step1;
>+      riscv_for_each_saved_reg (remaining_size, riscv_save_reg, false, false);
>     }
>
>   frame->mask = mask; /* Undo the above fib.  */
>@@ -5055,29 +5053,29 @@ riscv_expand_prologue (void)
>   if (frame_pointer_needed)
>     {
>       insn = gen_add3_insn (hard_frame_pointer_rtx, stack_pointer_rtx,
>-	    GEN_INT ((frame->hard_frame_pointer_offset - size).to_constant ()));
>+	    GEN_INT ((frame->hard_frame_pointer_offset - remaining_size).to_constant ()));
>       RTX_FRAME_RELATED_P (emit_insn (insn)) = 1;
>
>       riscv_emit_stack_tie ();
>     }
>
>   /* Allocate the rest of the frame.  */
>-  if (known_gt (size, 0))
>+  if (known_gt (remaining_size, 0))
>     {
>       /* Two step adjustment:
> 1.scalable frame. 2.constant frame.  */
>       poly_int64 scalable_frame (0, 0);
>-      if (!size.is_constant ())
>+      if (!remaining_size.is_constant ())
> {
>   /* First for scalable frame.  */
>-	  poly_int64 scalable_frame = size;
>-	  scalable_frame.coeffs[0] = size.coeffs[1];
>+	  poly_int64 scalable_frame = remaining_size;
>+	  scalable_frame.coeffs[0] = remaining_size.coeffs[1];
>   riscv_v_adjust_scalable_frame (stack_pointer_rtx, scalable_frame, false);
>-	  size -= scalable_frame;
>+	  remaining_size -= scalable_frame;
> }
>
>       /* Second step for constant frame.  */
>-      HOST_WIDE_INT constant_frame = size.to_constant ();
>+      HOST_WIDE_INT constant_frame = remaining_size.to_constant ();
>       if (constant_frame == 0)
> return;
>
>@@ -5142,6 +5140,8 @@ riscv_expand_epilogue (int style)
>   HOST_WIDE_INT step2 = 0;
>   bool use_restore_libcall = ((style == NORMAL_RETURN)
>       && riscv_use_save_libcall (frame));
>+  unsigned libcall_size = use_restore_libcall ?
>+                            frame->save_libcall_adjustment : 0;
>   rtx ra = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
>   rtx insn;
>
>@@ -5212,13 +5212,18 @@ riscv_expand_epilogue (int style)
>       REG_NOTES (insn) = dwarf;
>     }
>
>+  if (use_restore_libcall)
>+    frame->mask = 0; /* Temporarily fib for GPRs.  */
>+
>   /* If we need to restore registers, deallocate as much stack as
>      possible in the second step without going out of range.  */
>   if ((frame->mask | frame->fmask) != 0)
>-    {
>-      step2 = riscv_first_stack_step (frame, frame->total_size);
>-      step1 -= step2;
>-    }
>+    step2 = riscv_first_stack_step (frame, frame->total_size - libcall_size);
>+
>+  if (use_restore_libcall)
>+    frame->mask = mask; /* Undo the above fib.  */
>+
>+  step1 -= step2 + libcall_size;
>
>   /* Set TARGET to BASE + STEP1.  */
>   if (known_gt (step1, 0))
>@@ -5272,15 +5277,12 @@ riscv_expand_epilogue (int style)
>     frame->mask = 0; /* Temporarily fib that we need not save GPRs.  */
>
>   /* Restore the registers.  */
>-  riscv_for_each_saved_reg (frame->total_size - step2, riscv_restore_reg,
>+  riscv_for_each_saved_reg (frame->total_size - step2 - libcall_size,
>+                            riscv_restore_reg,
>     true, style == EXCEPTION_RETURN);
>
>   if (use_restore_libcall)
>-    {
>       frame->mask = mask; /* Undo the above fib.  */
>-      gcc_assert (step2 >= frame->save_libcall_adjustment);
>-      step2 -= frame->save_libcall_adjustment;
>-    }
>
>   if (need_barrier_p)
>     riscv_emit_stack_tie ();
>diff --git a/gcc/testsuite/gcc.target/riscv/stack_save_restore.c b/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
>new file mode 100644
>index 00000000000..522e706cfbf
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
>@@ -0,0 +1,40 @@
>+/* { dg-do compile } */
>+/* { dg-options "-march=rv32imafc -mabi=ilp32f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto" } */
>+/* { dg-final { check-function-bodies "**" "" } } */
>+
>+char my_getchar();
>+float getf();
>+
>+/*
>+**bar:
>+**	call	t0,__riscv_save_4
>+**	addi	sp,sp,-2032
>+**	...
>+**	li	t0,-12288
>+**	add	sp,sp,t0
>+**	...
>+**	li	t0,12288
>+**	add	sp,sp,t0
>+**	...
>+**	addi	sp,sp,2032
>+**	tail	__riscv_restore_4
>+*/
>+int bar()
>+{
>+  float volatile farray[3568];
>+
>+  float sum = 0;
>+  float f1 = getf();
>+  float f2 = getf();
>+  float f3 = getf();
>+  float f4 = getf();
>+
>+  for (int i = 0; i < 3568; i++)
>+  {
>+    farray[i] = my_getchar() * 1.2;
>+    sum += farray[i];
>+  }
>+
>+  return sum + f1 + f2 + f3 + f4;
>+}
>+
>--
>2.17.1

Re: [PATCH 2/3] RISC-V: optimize stack manipulation in save-restore

[Jeff Law]

On 12/1/22 03:03, Fei Gao wrote:
> The stack that save-restore reserves is not well accumulated in stack allocation and deallocation.
> This patch allows less instructions to be used in stack allocation and deallocation if save-restore enabled.
Haha!  I should have read the whole series before commenting on the 
first patch.  I think this addresses the precise issue I was asking 
about in my prior message.

Jeff

Re: [PATCH 2/3] RISC-V: optimize stack manipulation in save-restore

[Jeff Law]

On 12/1/22 03:03, Fei Gao wrote:
> The stack that save-restore reserves is not well accumulated in stack allocation and deallocation.
> This patch allows less instructions to be used in stack allocation and deallocation if save-restore enabled.
> 
> before patch:
>    bar:
>      call	t0,__riscv_save_4
>      addi	sp,sp,-64
>      ...
>      li	t0,-12288
>      addi	t0,t0,-1968 # optimized out after patch
>      add	sp,sp,t0 # prologue
>      ...
>      li	t0,12288 # epilogue
>      addi	t0,t0,2000 # optimized out after patch
>      add	sp,sp,t0
>      ...
>      addi	sp,sp,32
>      tail	__riscv_restore_4
> 
> after patch:
>    bar:
>      call	t0,__riscv_save_4
>      addi	sp,sp,-2032
>      ...
>      li	t0,-12288
>      add	sp,sp,t0 # prologue
>      ...
>      li	t0,12288 # epilogue
>      add	sp,sp,t0
>      ...
>      addi	sp,sp,2032
>      tail	__riscv_restore_4
> 
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_expand_prologue): consider save-restore in stack allocation.
>          (riscv_expand_epilogue): consider save-restore in stack deallocation.
> 
> gcc/testsuite/ChangeLog:
> 
>          * gcc.target/riscv/stack_save_restore.c: New test.
I made a couple of whitespace fixes and pushed this to the trunk after 
running it through a cross testing cycle.

Thanks!

jeff

[PATCH 3/3] RISC-V: make the stack manipulation codes more readable.

[Fei Gao]

gcc/ChangeLog:

        * config/riscv/riscv.cc (riscv_first_stack_step): make codes more readable.
        (riscv_expand_epilogue): likewise.
---
 gcc/config/riscv/riscv.cc | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index a50f2303032..95da08ffb3b 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -4926,8 +4926,11 @@ riscv_first_stack_step (struct riscv_frame_info *frame, poly_int64 remaining_siz
   if (SMALL_OPERAND (remaining_const_size))
     return remaining_const_size;
 
+  poly_int64 callee_saved_first_step =
+    remaining_size - frame->frame_pointer_offset;
+  gcc_assert(callee_saved_first_step.is_constant ());
   HOST_WIDE_INT min_first_step =
-    riscv_stack_align ((remaining_size - frame->frame_pointer_offset).to_constant());
+    riscv_stack_align (callee_saved_first_step.to_constant ());
   HOST_WIDE_INT max_first_step = IMM_REACH / 2 - PREFERRED_STACK_BOUNDARY / 8;
   HOST_WIDE_INT min_second_step = remaining_const_size - max_first_step;
   gcc_assert (min_first_step <= max_first_step);
@@ -4935,7 +4938,7 @@ riscv_first_stack_step (struct riscv_frame_info *frame, poly_int64 remaining_siz
   /* As an optimization, use the least-significant bits of the total frame
      size, so that the second adjustment step is just LUI + ADD.  */
   if (!SMALL_OPERAND (min_second_step)
-      && remaining_const_size % IMM_REACH < IMM_REACH / 2
+      && remaining_const_size % IMM_REACH <= max_first_step
       && remaining_const_size % IMM_REACH >= min_first_step)
     return remaining_const_size % IMM_REACH;
 
@@ -5129,14 +5132,14 @@ riscv_adjust_libcall_cfi_epilogue ()
 void
 riscv_expand_epilogue (int style)
 {
-  /* Split the frame into two.  STEP1 is the amount of stack we should
-     deallocate before restoring the registers.  STEP2 is the amount we
-     should deallocate afterwards.
+  /* Split the frame into 3 steps. STEP1 is the amount of stack we should
+     deallocate before restoring the registers. STEP2 is the amount we
+     should deallocate afterwards including the callee saved regs. STEP3
+     is the amount deallocated by save-restore libcall.
 
      Start off by assuming that no registers need to be restored.  */
   struct riscv_frame_info *frame = &cfun->machine->frame;
   unsigned mask = frame->mask;
-  poly_int64 step1 = frame->total_size;
   HOST_WIDE_INT step2 = 0;
   bool use_restore_libcall = ((style == NORMAL_RETURN)
 			      && riscv_use_save_libcall (frame));
@@ -5223,7 +5226,7 @@ riscv_expand_epilogue (int style)
   if (use_restore_libcall)
     frame->mask = mask; /* Undo the above fib.  */
 
-  step1 -= step2 + libcall_size;
+  poly_int64 step1 = frame->total_size - step2 - libcall_size;
 
   /* Set TARGET to BASE + STEP1.  */
   if (known_gt (step1, 0))
-- 
2.17.1

[PING] [PATCH 3/3] RISC-V: make the stack manipulation codes more readable.

[Fei Gao]

Gentle ping.

The patch I previously submitted:
| Date: Wed, 30 Nov 2022 00:38:08 -0800
| Subject: [PATCH] RISC-V: optimize stack manipulation in save-restore
| Message-ID: <gaofei@eswincomputing.com>

I split the patches as per Palmer's review comment.

BR
Fei

>gcc/ChangeLog:
>
>        * config/riscv/riscv.cc (riscv_first_stack_step): make codes more readable.
>        (riscv_expand_epilogue): likewise.
>---
> gcc/config/riscv/riscv.cc | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
>diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>index a50f2303032..95da08ffb3b 100644
>--- a/gcc/config/riscv/riscv.cc
>+++ b/gcc/config/riscv/riscv.cc
>@@ -4926,8 +4926,11 @@ riscv_first_stack_step (struct riscv_frame_info *frame, poly_int64 remaining_siz
>   if (SMALL_OPERAND (remaining_const_size))
>     return remaining_const_size;
>
>+  poly_int64 callee_saved_first_step =
>+    remaining_size - frame->frame_pointer_offset;
>+  gcc_assert(callee_saved_first_step.is_constant ());
>   HOST_WIDE_INT min_first_step =
>-    riscv_stack_align ((remaining_size - frame->frame_pointer_offset).to_constant());
>+    riscv_stack_align (callee_saved_first_step.to_constant ());
>   HOST_WIDE_INT max_first_step = IMM_REACH / 2 - PREFERRED_STACK_BOUNDARY / 8;
>   HOST_WIDE_INT min_second_step = remaining_const_size - max_first_step;
>   gcc_assert (min_first_step <= max_first_step);
>@@ -4935,7 +4938,7 @@ riscv_first_stack_step (struct riscv_frame_info *frame, poly_int64 remaining_siz
>   /* As an optimization, use the least-significant bits of the total frame
>      size, so that the second adjustment step is just LUI + ADD.  */
>   if (!SMALL_OPERAND (min_second_step)
>-      && remaining_const_size % IMM_REACH < IMM_REACH / 2
>+      && remaining_const_size % IMM_REACH <= max_first_step
>       && remaining_const_size % IMM_REACH >= min_first_step)
>     return remaining_const_size % IMM_REACH;
>
>@@ -5129,14 +5132,14 @@ riscv_adjust_libcall_cfi_epilogue ()
> void
> riscv_expand_epilogue (int style)
> {
>-  /* Split the frame into two.  STEP1 is the amount of stack we should
>-     deallocate before restoring the registers.  STEP2 is the amount we
>-     should deallocate afterwards.
>+  /* Split the frame into 3 steps. STEP1 is the amount of stack we should
>+     deallocate before restoring the registers. STEP2 is the amount we
>+     should deallocate afterwards including the callee saved regs. STEP3
>+     is the amount deallocated by save-restore libcall.
>
>      Start off by assuming that no registers need to be restored.  */
>   struct riscv_frame_info *frame = &cfun->machine->frame;
>   unsigned mask = frame->mask;
>-  poly_int64 step1 = frame->total_size;
>   HOST_WIDE_INT step2 = 0;
>   bool use_restore_libcall = ((style == NORMAL_RETURN)
>       && riscv_use_save_libcall (frame));
>@@ -5223,7 +5226,7 @@ riscv_expand_epilogue (int style)
>   if (use_restore_libcall)
>     frame->mask = mask; /* Undo the above fib.  */
>
>-  step1 -= step2 + libcall_size;
>+  poly_int64 step1 = frame->total_size - step2 - libcall_size;
>
>   /* Set TARGET to BASE + STEP1.  */
>   if (known_gt (step1, 0))
>--
>2.17.1

Re: [PATCH 3/3] RISC-V: make the stack manipulation codes more readable.

[Jeff Law]

On 12/1/22 03:03, Fei Gao wrote:
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_first_stack_step): make codes more readable.
>          (riscv_expand_epilogue): likewise.
Thanks.  I fixed up the capitalization and one or two whitespace errors 
and pushed this patch to the trunk.

Thanks for your patience.

jeff

[PING] [PATCH 0/3] RISC-V: optimize stack manipulation in save-restore

[Fei Gao]

Gentle ping.

The patch I previously submitted:
| Date: Wed, 30 Nov 2022 00:38:08 -0800
| Subject: [PATCH] RISC-V: optimize stack manipulation in save-restore
| Message-ID: <gaofei@eswincomputing.com>

I split the patches as per Palmer's review comment.

BR
Fei

On 2022-12-01 18:03  Fei Gao <gaofei@eswincomputing.com> wrote:
>
>The patches allow less instructions to be used in stack allocation
>and deallocation if save-restore enabled, and also make the stack
>manipulation codes more readable.
>
>Fei Gao (3):
>  RISC-V: add a new parameter in riscv_first_stack_step.
>  RISC-V: optimize stack manipulation in save-restore
>  RISC-V: make the stack manipulation codes more readable.
>
> gcc/config/riscv/riscv.cc                     | 105 +++++++++---------
> .../gcc.target/riscv/stack_save_restore.c     |  40 +++++++
> 2 files changed, 95 insertions(+), 50 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/stack_save_restore.c
>
>--
>2.17.1

[PING 2] [PATCH 0/3] RISC-V: optimize stack manipulation in save-restore

[Fei Gao]

ping.

BR
Fei

On 2023-02-03 16:52  Fei Gao <gaofei@eswincomputing.com> wrote:
>
>
>Gentle ping.
>
>The patch I previously submitted:
>| Date: Wed, 30 Nov 2022 00:38:08 -0800
>| Subject: [PATCH] RISC-V: optimize stack manipulation in save-restore
>| Message-ID: <gaofei@eswincomputing.com>
>
>I split the patches as per Palmer's review comment.
>
>BR
>Fei
>
>On 2022-12-01 18:03  Fei Gao <gaofei@eswincomputing.com> wrote:
>>
>>The patches allow less instructions to be used in stack allocation
>>and deallocation if save-restore enabled, and also make the stack
>>manipulation codes more readable.
>>
>>Fei Gao (3):
>>  RISC-V: add a new parameter in riscv_first_stack_step.
>>  RISC-V: optimize stack manipulation in save-restore
>>  RISC-V: make the stack manipulation codes more readable.
>>
>> gcc/config/riscv/riscv.cc                     | 105 +++++++++---------
>> .../gcc.target/riscv/stack_save_restore.c     |  40 +++++++
>> 2 files changed, 95 insertions(+), 50 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.target/riscv/stack_save_restore.c
>>
>>--
>>2.17.1

Re: [PING 2] [PATCH 0/3] RISC-V: optimize stack manipulation in save-restore

[Fei Gao]

ping.

BR, 
Fei

On 2023-02-03 16:52  Fei Gao <gaofei@eswincomputing.com> wrote:
>
>
>Gentle ping.
>
>The patch I previously submitted:
>| Date: Wed, 30 Nov 2022 00:38:08 -0800
>| Subject: [PATCH] RISC-V: optimize stack manipulation in save-restore
>| Message-ID: <gaofei@eswincomputing.com>
>
>I split the patches as per Palmer's review comment.
>
>BR
>Fei
>
>On 2022-12-01 18:03  Fei Gao <gaofei@eswincomputing.com> wrote:
>>
>>The patches allow less instructions to be used in stack allocation
>>and deallocation if save-restore enabled, and also make the stack
>>manipulation codes more readable.
>>
>>Fei Gao (3):
>>  RISC-V: add a new parameter in riscv_first_stack_step.
>>  RISC-V: optimize stack manipulation in save-restore
>>  RISC-V: make the stack manipulation codes more readable.
>>
>> gcc/config/riscv/riscv.cc                     | 105 +++++++++---------
>> .../gcc.target/riscv/stack_save_restore.c     |  40 +++++++
>> 2 files changed, 95 insertions(+), 50 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.target/riscv/stack_save_restore.c
>>
>>--
>>2.17.1

Re: [PING 2] [PATCH 0/3] RISC-V: optimize stack manipulation in save-restore

[Jeff Law]

On 2/16/23 00:17, Fei Gao wrote:
> ping.
We are in stage4 of our development cycle -- meaning that the focus is 
on regression bugfixing, not new features, optimizations and the like.

This patch is in the queue and will be looked at once we move back into 
stage1 development for gcc-14.

jeff