RE: [PATCH] RISCV: Add -m(no)-omit-leaf-frame-pointer support.

["Wang, Yanzhang" <yanzhang.wang@intel.com>]

> +static bool
> +riscv_frame_pointer_required (void)
> +{
> +  if (riscv_save_frame_pointer && !crtl->is_leaf)
> +    return true;
> +
> +  return false;
> +}
> 
> Can be simplified to return riscv_save_frame_pointer && !crtl->is_leaf;

Nice. It's much simpler. Will modify in another patch.

> +  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;
> +    }
> 
> Does this mean if omit_leaf_frame will also set the omit_frame_pointer
> implicitly?
>

For the flag it's yes but for the behavior it's no. The behavior still is
based on the flag of omit-frame-pointer's value.

- ON, than the frame pointer of non-leaf functions will be omitted.
- OFF(no), than the frame pointer of non-leaf functions will not be omitted.

In the other words, if we want to omit the leaf frame pointers,

- if we want to omit the non-leaf fp too, we need only save the ra for the non-leaf.
- if we don't, we need to save the fp+ra for the non-leaf but no fp+ra for the leaf.

We need to override the option (x_flag_omit_frame_pointer) because it's the
first priority when determine whether the frame pointer is needed. If it's
turned off, the frame pointer will be saved for leaf functions too even
though we turn on the omit-leaf-frame-pointer.

To distinguish the two scenarios above, we need to add another variable to
save the flag user set originally otherwise it will be threw away.

Yanzhang

> -----Original Message-----
> From: Li, Pan2 <pan2.li@intel.com>
> Sent: Monday, June 5, 2023 9:04 AM
> To: Wang, Yanzhang <yanzhang.wang@intel.com>; gcc-patches@gcc.gnu.org
> Cc: juzhe.zhong@rivai.ai; kito.cheng@sifive.com
> Subject: RE: [PATCH] RISCV: Add -m(no)-omit-leaf-frame-pointer support.
> 
> Some nit comments.
> 
> +static bool
> +riscv_frame_pointer_required (void)
> +{
> +  if (riscv_save_frame_pointer && !crtl->is_leaf)
> +    return true;
> +
> +  return false;
> +}
> 
> Can be simplified to return riscv_save_frame_pointer && !crtl->is_leaf;
> 
> +  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;
> +    }
> 
> Does this mean if omit_leaf_frame will also set the omit_frame_pointer
> implicitly?
> 
> Pan
> 
> 
> -----Original Message-----
> From: Wang, Yanzhang <yanzhang.wang@intel.com>
> Sent: Friday, June 2, 2023 3:07 PM
> To: gcc-patches@gcc.gnu.org
> Cc: juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Li, Pan2
> <pan2.li@intel.com>; Wang, Yanzhang <yanzhang.wang@intel.com>
> Subject: [PATCH] RISCV: Add -m(no)-omit-leaf-frame-pointer support.
> 
> 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>
> ---
>  gcc/config/riscv/riscv.cc                     | 31 ++++++++++++++++++-
>  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, 75 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
> 
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index
> 5d2550871c7..e02f9cb50a4 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;
> @@ -4744,7 +4748,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.
> */ @@ -6287,6 +6295,15 @@ riscv_option_override (void)
>    if (flag_pic)
>      riscv_cmodel = CM_PIC;
> 
> +  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) @@ -7158,6
> +7175,15 @@ riscv_zero_call_used_regs (HARD_REG_SET need_zeroed_hardregs)
>  							& ~zeroed_hardregs);
>  }
> 
> +static bool
> +riscv_frame_pointer_required (void)
> +{
> +  if (riscv_save_frame_pointer && !crtl->is_leaf)
> +    return true;
> +
> +  return false;
> +}
> +
>  /* Initialize the GCC target structure.  */  #undef
> TARGET_ASM_ALIGNED_HI_OP  #define TARGET_ASM_ALIGNED_HI_OP "\t.half\t"
> @@ -7412,6 +7438,9 @@ riscv_zero_call_used_regs (HARD_REG_SET
> need_zeroed_hardregs)  #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 ff1dd4ddd4f..c3e2e7c1da4 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;
> +}
> --
> 2.40.1