diff mbox series

[gcc-13-backport] RISCV: Add -m(no)-omit-leaf-frame-pointer support.

Message ID 20240508173219.20808-2-palmer@rivosinc.com
State New
Headers
Series [gcc-13-backport] RISCV: Add -m(no)-omit-leaf-frame-pointer support. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 fail Patch failed to apply

Commit Message

Palmer Dabbelt May 8, 2024, 5:32 p.m. UTC 
  From: Yanzhang Wang <yanzhang.wang@intel.com>

gcc/ChangeLog:

	* config/riscv/riscv.cc (riscv_save_reg_p): Save ra for leaf
	when enabling -mno-omit-leaf-frame-pointer
	(riscv_option_override): Override omit-frame-pointer.
	(riscv_frame_pointer_required): Save s0 for non-leaf function
	(TARGET_FRAME_POINTER_REQUIRED): Override defination
	* config/riscv/riscv.opt: Add option support.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/omit-frame-pointer-1.c: New test.
	* gcc.target/riscv/omit-frame-pointer-2.c: New test.
	* gcc.target/riscv/omit-frame-pointer-3.c: New test.
	* gcc.target/riscv/omit-frame-pointer-4.c: New test.
	* gcc.target/riscv/omit-frame-pointer-test.c: New test.

Signed-off-by: Yanzhang Wang <yanzhang.wang@intel.com>
(cherry picked from commit 39663298b5934831a0125e12f113ebd83248c3be)
---
I haven't tested this (just an all-gcc build), but I figured I'd just
send it now as it's kind of a grey area for backports: the flag itself
is a new feature, but it also fixes a compatibility issue with the psABI
-- which itself is a grey area, as the psABI change was a retrofit and is
marked as optional.  I'd test it before pushing it, but this is one of
those things where I'm not really sure what the backporting rules
indicate we should do.

There's more discussion on this LKML thread:
https://lore.kernel.org/linux-riscv/527dd4d8-f1e5-4581-b1e3-aa315fea8caf@sifive.com/T/#mf15ccc659b7b8b838b88959fbea460210875eb9c

That also has a much smaller fix, but having the whole argument seems
like a nicer user interface to me -- then users who really want
compatibility with the psABI's section on frame records can just ask for
it directly (via the odd spelling `-fno-omit-frame-pointer
-mno-omit-leaf-frame-pointer`, but too late to change that).

Thoughts on this for 13?

We'd probably also want it all the way back to 11, but I assume that's
going to be the same discussion.
---
 gcc/config/riscv/riscv.cc                     | 34 ++++++++++++++++++-
 gcc/config/riscv/riscv.opt                    |  4 +++
 .../gcc.target/riscv/omit-frame-pointer-1.c   |  7 ++++
 .../gcc.target/riscv/omit-frame-pointer-2.c   |  7 ++++
 .../gcc.target/riscv/omit-frame-pointer-3.c   |  7 ++++
 .../gcc.target/riscv/omit-frame-pointer-4.c   |  7 ++++
 .../riscv/omit-frame-pointer-test.c           | 13 +++++++
 7 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/omit-frame-pointer-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/omit-frame-pointer-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/omit-frame-pointer-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/omit-frame-pointer-4.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/omit-frame-pointer-test.c

Comments

Jeff Law May 8, 2024, 10:19 p.m. UTC | #1 
On 5/8/24 11:32 AM, Palmer Dabbelt wrote:
> From: Yanzhang Wang <yanzhang.wang@intel.com>
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/riscv.cc (riscv_save_reg_p): Save ra for leaf
> 	when enabling -mno-omit-leaf-frame-pointer
> 	(riscv_option_override): Override omit-frame-pointer.
> 	(riscv_frame_pointer_required): Save s0 for non-leaf function
> 	(TARGET_FRAME_POINTER_REQUIRED): Override defination
> 	* config/riscv/riscv.opt: Add option support.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/omit-frame-pointer-1.c: New test.
> 	* gcc.target/riscv/omit-frame-pointer-2.c: New test.
> 	* gcc.target/riscv/omit-frame-pointer-3.c: New test.
> 	* gcc.target/riscv/omit-frame-pointer-4.c: New test.
> 	* gcc.target/riscv/omit-frame-pointer-test.c: New test.
> 
> Signed-off-by: Yanzhang Wang <yanzhang.wang@intel.com>
> (cherry picked from commit 39663298b5934831a0125e12f113ebd83248c3be)
> ---
> I haven't tested this (just an all-gcc build), but I figured I'd just
> send it now as it's kind of a grey area for backports: the flag itself
> is a new feature, but it also fixes a compatibility issue with the psABI
> -- which itself is a grey area, as the psABI change was a retrofit and is
> marked as optional.  I'd test it before pushing it, but this is one of
> those things where I'm not really sure what the backporting rules
> indicate we should do.
> 
> There's more discussion on this LKML thread:
> https://lore.kernel.org/linux-riscv/527dd4d8-f1e5-4581-b1e3-aa315fea8caf@sifive.com/T/#mf15ccc659b7b8b838b88959fbea460210875eb9c
> 
> That also has a much smaller fix, but having the whole argument seems
> like a nicer user interface to me -- then users who really want
> compatibility with the psABI's section on frame records can just ask for
> it directly (via the odd spelling `-fno-omit-frame-pointer
> -mno-omit-leaf-frame-pointer`, but too late to change that).
> 
> Thoughts on this for 13?
Given its target specific, I think we have a lot more leeway here.

I think there was a followup in the space.  defa8681d951


> 
> We'd probably also want it all the way back to 11, but I assume that's
> going to be the same discussion.
Yea.

You might explicitly run it by Jakub.  But I'm certainly OK with this 
being backported.

jeff
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index cefd3b7b2b2..e8572f8739d 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -408,6 +408,10 @@  static const struct riscv_tune_info riscv_tune_info_table[] = {
 #include "riscv-cores.def"
 };
 
+/* Global variable to distinguish whether we should save and restore s0/fp for
+   function.  */
+static bool riscv_save_frame_pointer;
+
 void riscv_frame_info::reset(void)
 {
   total_size = 0;
@@ -4786,7 +4790,11 @@  riscv_save_reg_p (unsigned int regno)
   if (regno == HARD_FRAME_POINTER_REGNUM && frame_pointer_needed)
     return true;
 
-  if (regno == RETURN_ADDR_REGNUM && crtl->calls_eh_return)
+  /* Need not to use ra for leaf when frame pointer is turned off by option
+     whatever the omit-leaf-frame's value.  */
+  bool keep_leaf_ra = frame_pointer_needed && crtl->is_leaf
+    && !TARGET_OMIT_LEAF_FRAME_POINTER;
+  if (regno == RETURN_ADDR_REGNUM && (crtl->calls_eh_return || keep_leaf_ra))
     return true;
 
   /* If this is an interrupt handler, then must save extra registers.  */
@@ -6316,6 +6324,21 @@  riscv_option_override (void)
   if (flag_pic)
     riscv_cmodel = CM_PIC;
 
+  /* We need to save the fp with ra for non-leaf functions with no fp and ra
+     for leaf functions while no-omit-frame-pointer with
+     omit-leaf-frame-pointer.  The x_flag_omit_frame_pointer has the first
+     priority to determine whether the frame pointer is needed.  If we do not
+     override it, the fp and ra will be stored for leaf functions, which is not
+     our wanted.  */
+  riscv_save_frame_pointer = false;
+  if (TARGET_OMIT_LEAF_FRAME_POINTER_P (global_options.x_target_flags))
+    {
+      if (!global_options.x_flag_omit_frame_pointer)
+	riscv_save_frame_pointer = true;
+
+      global_options.x_flag_omit_frame_pointer = 1;
+    }
+
   /* We get better code with explicit relocs for CM_MEDLOW, but
      worse code for the others (for now).  Pick the best default.  */
   if ((target_flags_explicit & MASK_EXPLICIT_RELOCS) == 0)
@@ -7235,6 +7258,12 @@  riscv_lshift_subword (machine_mode mode, rtx value, rtx shift,
 						  gen_lowpart (QImode, shift)));
 }
 
+static bool
+riscv_frame_pointer_required (void)
+{
+  return riscv_save_frame_pointer && !crtl->is_leaf;
+}
+
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP
 #define TARGET_ASM_ALIGNED_HI_OP "\t.half\t"
@@ -7489,6 +7518,9 @@  riscv_lshift_subword (machine_mode mode, rtx value, rtx shift,
 #undef TARGET_ZERO_CALL_USED_REGS
 #define TARGET_ZERO_CALL_USED_REGS riscv_zero_call_used_regs
 
+#undef TARGET_FRAME_POINTER_REQUIRED
+#define TARGET_FRAME_POINTER_REQUIRED riscv_frame_pointer_required
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-riscv.h"
diff --git a/gcc/config/riscv/riscv.opt b/gcc/config/riscv/riscv.opt
index bc5e63ab3e6..7f291aadb7d 100644
--- a/gcc/config/riscv/riscv.opt
+++ b/gcc/config/riscv/riscv.opt
@@ -138,6 +138,10 @@  Enable the CSR checking for the ISA-dependent CRS and the read-only CSR.
 The ISA-dependent CSR are only valid when the specific ISA is set.  The
 read-only CSR can not be written by the CSR instructions.
 
+momit-leaf-frame-pointer
+Target Mask(OMIT_LEAF_FRAME_POINTER) Save
+Omit the frame pointer in leaf functions.
+
 Mask(64BIT)
 
 Mask(MUL)
diff --git a/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-1.c b/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-1.c
new file mode 100644
index 00000000000..c96123ea702
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-1.c
@@ -0,0 +1,7 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64 -O2 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fno-inline" } */
+
+#include "omit-frame-pointer-test.c"
+
+/* { dg-final { scan-assembler-times "sd\tra,\[0-9\]+\\(sp\\)" 2 } } */
+/* { dg-final { scan-assembler-times "sd\ts0,\[0-9\]+\\(sp\\)" 2 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-2.c b/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-2.c
new file mode 100644
index 00000000000..067148c6a58
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-2.c
@@ -0,0 +1,7 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64 -O2 -fno-omit-frame-pointer -momit-leaf-frame-pointer -fno-inline" } */
+
+#include "omit-frame-pointer-test.c"
+
+/* { dg-final { scan-assembler-times "sd\tra,\[0-9\]+\\(sp\\)" 1 } } */
+/* { dg-final { scan-assembler-times "sd\ts0,\[0-9\]+\\(sp\\)" 1 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-3.c b/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-3.c
new file mode 100644
index 00000000000..b4d7d6f4f0d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-3.c
@@ -0,0 +1,7 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64 -O2 -fomit-frame-pointer -mno-omit-leaf-frame-pointer -fno-inline" } */
+
+#include "omit-frame-pointer-test.c"
+
+/* { dg-final { scan-assembler-times "sd\tra,\[0-9\]+\\(sp\\)" 1 } } */
+/* { dg-final { scan-assembler-not "sd\ts0,\[0-9\]+\\(sp\\)"} } */
diff --git a/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-4.c b/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-4.c
new file mode 100644
index 00000000000..5a5b540ef4e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-4.c
@@ -0,0 +1,7 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64 -O2 -fomit-frame-pointer -momit-leaf-frame-pointer -fno-inline" } */
+
+#include "omit-frame-pointer-test.c"
+
+/* { dg-final { scan-assembler-times "sd\tra,\[0-9\]+\\(sp\\)" 1 } } */
+/* { dg-final { scan-assembler-not "sd\ts0,\[0-9\]+\\(sp\\)"} } */
diff --git a/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-test.c b/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-test.c
new file mode 100644
index 00000000000..cf19f001e29
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/omit-frame-pointer-test.c
@@ -0,0 +1,13 @@ 
+int inc(int n)
+{
+  return n + 1;
+}
+
+
+int bar(void)
+{
+  int n = 100;
+  n = inc(n);
+  n = inc(n) + 100;
+  return n;
+}