[2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0

When using the -msave-restore flag we end up with calls to
_riscv_save_0 and _riscv_restore_0.  These functions adjust the stack
and save or restore the return address.  Due to grouping multiple
save/restore stub functions together the save/restore 0 calls actually
save s0, s1, s2, and the return address, but only the return address
actually matters.  Leaf functions don't call the save/restore stubs,
so whenever we do see a call to the save/restore stubs, the store of
the return address is required.

If we look in gcc/config/riscv/riscv.c at the function
riscv_expand_prologue and riscv_expand_epilogue we can see that it
would be reasonably easy to adjust these functions to avoid the calls
to the save/restore stubs for those cases where we are about to call
_riscv_save_0 and _riscv_restore_0, however, the actual code size
saving this would give is debatable, with linker relaxation, the calls
to save/restore are often just 4-bytes, and can sometimes even be
2-bytes, while leaving the stack adjust and return address save inline
is always going to be 4-bytes.

The interesting case is when we call _riscv_save_0 and
_riscv_restore_0, and also have a frame that would (without
save/restore) have resulted in a tail call.  In this case if we could
remove the save/restore calls, and restore the tail call then we would
get a real size saving.

The problem is that the choice of generating a tail call or not is
done during the gimple expand pass, at which point we don't know how
many registers we need to save (or restore).

The solution presented in this patch offers a partial solution to this
problem.  By using the TARGET_MACHINE_DEPENDENT_REORG pass to
implement a very limited pattern matching we identify functions that
call _riscv_save_0 and _riscv_restore_0, and which could be converted
to make use of a tail call.  These functions are then converted to the
non save/restore tail call form.

This should result in a code size reduction when compiling with -Os
and with the -msave-restore flag.

gcc/ChangeLog:

	* config.gcc: Add riscv-sr.o to extra_objs for riscv.
	* config/riscv/riscv-sr.c: New file.
	* config/riscv/riscv.c (riscv_reorg): New function.
	(TARGET_MACHINE_DEPENDENT_REORG): Define.
	* config/riscv/riscv.h (SIBCALL_REG_P): Define.
	(CALLEE_SAVED_REG_P): Define.
	(riscv_remove_unneeded_save_restore_calls): Declare.
	* config/riscv/t-riscv (riscv-sr.o): New build rule.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/save-restore-2.c: New file.
	* gcc.target/riscv/save-restore-3.c: New file.
	* gcc.target/riscv/save-restore-4.c: New file.
	* gcc.target/riscv/save-restore-5.c: New file.
	* gcc.target/riscv/save-restore-6.c: New file.
	* gcc.target/riscv/save-restore-7.c: New file.
	* gcc.target/riscv/save-restore-8.c: New file.
---
 gcc/ChangeLog                                   |  11 +
 gcc/config.gcc                                  |   2 +-
 gcc/config/riscv/riscv-sr.c                     | 375 ++++++++++++++++++++++++
 gcc/config/riscv/riscv.c                        |  13 +
 gcc/config/riscv/riscv.h                        |  20 ++
 gcc/config/riscv/t-riscv                        |   5 +
 gcc/testsuite/ChangeLog                         |  10 +
 gcc/testsuite/gcc.target/riscv/save-restore-2.c |  22 ++
 gcc/testsuite/gcc.target/riscv/save-restore-3.c |  16 +
 gcc/testsuite/gcc.target/riscv/save-restore-4.c |  27 ++
 gcc/testsuite/gcc.target/riscv/save-restore-5.c |   9 +
 gcc/testsuite/gcc.target/riscv/save-restore-6.c |  16 +
 gcc/testsuite/gcc.target/riscv/save-restore-7.c |  30 ++
 gcc/testsuite/gcc.target/riscv/save-restore-8.c |  12 +
 14 files changed, 567 insertions(+), 1 deletion(-)
 create mode 100644 gcc/config/riscv/riscv-sr.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/save-restore-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/save-restore-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/save-restore-4.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/save-restore-5.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/save-restore-6.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/save-restore-7.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/save-restore-8.c

On Mon, Aug 19, 2019 at 12:15 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
>         * config.gcc: Add riscv-sr.o to extra_objs for riscv.
>         * config/riscv/riscv-sr.c: New file.
>         * config/riscv/riscv.c (riscv_reorg): New function.
>         (TARGET_MACHINE_DEPENDENT_REORG): Define.
>         * config/riscv/riscv.h (SIBCALL_REG_P): Define.
>         (CALLEE_SAVED_REG_P): Define.
>         (riscv_remove_unneeded_save_restore_calls): Declare.
>         * config/riscv/t-riscv (riscv-sr.o): New build rule.

I haven't studied the code yet.  I hope to get to that soon.  I did
try testing it, as I can do that in the background.  For testing
purposes, I added code to riscv_option_override to enable
-msave-restore if there was no command line option specified for it.
  if ((target_flags_explicit & MASK_SAVE_RESTORE) == 0)
    target_flags |= MASK_SAVE_RESTORE;
I then did a toolchain build and check with and without the patch to
see the result.

For a rv32i/ilp32 newlib toolchain, I get 12 additional gcc testsuite
failures with the patch.  I think the last two are probably cases
where you patch eliminates the need to call
__riscv_save_0/__riscv_restore_0, so probably not a bug in your patch,
just a testcase that needs to be updated.
> FAIL: gcc.c-torture/execute/20041218-1.c   -Os  (internal compiler error)
> FAIL: gcc.c-torture/execute/20041218-1.c   -Os  (test for excess errors)
> FAIL: gcc.c-torture/execute/20111227-1.c   -O2  execution test
> FAIL: gcc.c-torture/execute/20111227-1.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
> FAIL: gcc.c-torture/execute/20111227-1.c   -O3 -g  execution test
> FAIL: gcc.dg/ipa/ipa-icf-31.c execution test
> FAIL: gcc.dg/torture/pr42878-2.c   -O1  (test for excess errors)
> FAIL: gcc.dg/torture/pr42878-2.c   -O2  (test for excess errors)
> FAIL: gcc.dg/torture/pr42878-2.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  (test for excess errors)
> FAIL: gcc.dg/torture/pr42878-2.c   -O3 -g  (test for excess errors)
> FAIL: gcc.target/riscv/save-restore-4.c scan-assembler call[ \t]*t0,__riscv_save_0
> FAIL: gcc.target/riscv/save-restore-4.c scan-assembler tail[ \t]*__riscv_restore_0
I get two additional failures for g++.
> FAIL: g++.dg/ipa/pr78211.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/ipa/pr78211.C  -std=gnu++17 (test for excess errors)

With a rv64gc/lp64d linux toolchain, I get 43 extra gcc failures.  For
C++ and Fortran I get a lot of link errors, so the results aren't very
useful.  Apparently we never tested this before.
/scratch/jimw/fsf-testing/X-tmp-unpatched-64/build-install/riscv64-unknown-linux-gnu/bin/ld:
linker_plugin30724.exe: hidden symbol `__riscv_save_0' in
/scratch/jimw/fsf-testing/X-tmp-unpatched-64/build-gcc-linux-stage2/gcc/testsuite/g++/../../libgcc.a(save-restore.o)
is referenced by DSO
Not sure why it is marked hidden, as we aren't using any visibility
directives.  Looks like I will have to add this to my todo list also.

Jim

On Mon, Aug 19, 2019 at 12:15 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> The solution presented in this patch offers a partial solution to this
> problem.  By using the TARGET_MACHINE_DEPENDENT_REORG pass to
> implement a very limited pattern matching we identify functions that
> call _riscv_save_0 and _riscv_restore_0, and which could be converted
> to make use of a tail call.  These functions are then converted to the
> non save/restore tail call form.

Looking at this patch, I noticed a typo "sone" -> "some".  You are
using the British spellings of optimise, optimising, and optimisation.

You have t0/x5 in SIBCALL_REG_P which I'd prefer to avoid because it
is the alternative return reg.  SIBCALL_REG_P looks like a potential
maintenance problem, as it is duplicating info already available in
REG_CLASS_CONTENTS and riscv_regno_to_class.  Maybe you can just use
riscv_regno_to_class to compute the info, that avoids adding another
copy of the info.

Similarly callee_saved_reg_p is duplicating info available elsewhere.
You can probably just check call_used_regs and check for a 0 value.

You have two loops to look for NOTE_INSN_PROLOGUE_END, but these will
be small functions so not necessarily a problem.

I'm wondering how well this works with debug info, since deleting the
save_0, restore_0, and call will be deleting REG_NOTES that have call
frame info, and you aren't adding anything back.  But it looks like
this works out OK as you aren't trying to change the frame, so the
info looks like it should still be right with those REG_NOTES deleted.
I didn't see anything obviously wrong here.

Otherwise, the basic logic looks fine.  It isn't obvious why it is
failing.  You will have to debug some of the failing testcases and
refine the patch to work for them.

I do see about a 1.5% size decrease for libc.a and a 2.5% size
decrease for libstdc++.a for a 32-bit newlib toolchain build with
-msave-restore hardwired on, with versus without your patch.  So it is
reducing code size.

Jim

On Thu, Aug 22, 2019 at 10:59 PM Jim Wilson <jimw@sifive.com> wrote:
> With a rv64gc/lp64d linux toolchain, I get 43 extra gcc failures.  For
> C++ and Fortran I get a lot of link errors, so the results aren't very
> useful.  Apparently we never tested this before.
> /scratch/jimw/fsf-testing/X-tmp-unpatched-64/build-install/riscv64-unknown-linux-gnu/bin/ld:
> linker_plugin30724.exe: hidden symbol `__riscv_save_0' in
> /scratch/jimw/fsf-testing/X-tmp-unpatched-64/build-gcc-linux-stage2/gcc/testsuite/g++/../../libgcc.a(save-restore.o)
> is referenced by DSO
> Not sure why it is marked hidden, as we aren't using any visibility
> directives.  Looks like I will have to add this to my todo list also.

The problem with the linux toolchain support for -msave-restore is
that we forgot to add a version script to export the symbols.  I made
the obvious change to add a RISC-V specific libgcc version script, and
now everything in the g++ and gfortran testsuites are linking, but the
execution tests are all timing out.  So there is still something
wrong.  I need to do some more work here.

Jim

On Sun, Aug 25, 2019 at 9:40 AM Jim Wilson <jimw@sifive.com> wrote:
> The problem with the linux toolchain support for -msave-restore is
> that we forgot to add a version script to export the symbols.  I made
> the obvious change to add a RISC-V specific libgcc version script, and
> now everything in the g++ and gfortran testsuites are linking, but the
> execution tests are all timing out.  So there is still something
> wrong.  I need to do some more work here.

Following up on this, I found a linker bug where it was emitting error
messages for non-pic code in shared libraries, but wasn't exiting with
an error, so bad shared libraries were being produced without killing
the build.  I wrote and committed a binutils patch to fix that.  The
underlying problem is that the save/restore functions were always
called as non-pic, even for a shared library.  But fixing that
produced shared libraries that failed again, which turned out because
the save/restore functions use the alternate link register t0/x5 which
is clobbered by plts, so we can't call them in shared libraries at
all.  I wrote and committed a gcc patch to disable -msave-restore when
-fpic is used, and emit a warning message if the user explicitly
turned on -msave-restore.  Since we can't use the save/restore
functions in shared libraries, we don't need to export them or support
pic calls to them, and I dropped those patches.  With these changes
I'm now able to do -msave-restore testing with a linux toolchain.
Testing with and without your patch for a riscv64-linux toolchain, I
see 11 extra gcc failures, 2 extra g++ failures, and 8 extra gfortran
failures.  I didn't look at the details.  I suspect that most of these
are failing for the same reason, and there is only a couple of minor
bugs that need to be fixed to make your patch work.

Jim

On Aug 30 2019, Jim Wilson <jimw@sifive.com> wrote:

> produced shared libraries that failed again, which turned out because
> the save/restore functions use the alternate link register t0/x5 which
> is clobbered by plts, so we can't call them in shared libraries at
> all.

Shouldn't the save/restore functions always be local to the object, thus
never be called through the PLT?

Andreas.

On Aug 30 2019, Jim Wilson <jimw@sifive.com> wrote:

> underlying problem is that the save/restore functions were always
> called as non-pic, even for a shared library.  But fixing that
> produced shared libraries that failed again, which turned out because
> the save/restore functions use the alternate link register t0/x5 which
> is clobbered by plts, so we can't call them in shared libraries at
> all.

I think the problem is that riscv needs to use t-slibgcc-libgcc so that
libgcc_s.so is a linker script.

Andreas.

On Sat, Aug 31, 2019 at 1:08 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> I think the problem is that riscv needs to use t-slibgcc-libgcc so that
> libgcc_s.so is a linker script.

Yes, thanks, that fixes the problem and works well.  I've got a patch
using this solution now.

Jim

Below is a new versions of this patch, I believe that this addresses
the review comments from the earlier version.  In addition this has
been tested using Jim's idea of forcing -msave-restore (if the flag is
not otherwise given) and I now see no test failures for newlib and
linux toolchains.

One thing that has been mentioned briefly was adding a flag to control
just this optimisation, I haven't done that yet, but can if that is
required - obviously this optimisation doesn't do anything if
-msave-restore is turned off, so that is always an option.  With no
test failures I don't know if a separate flag is required.

Thanks,
Andrew

---

commit f1b824e94f3ea396bd0ef46692d5211f855d4b4c
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Thu Jul 18 16:03:10 2019 +0100

    gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
    
    When using the -msave-restore flag we end up with calls to
    _riscv_save_0 and _riscv_restore_0.  These functions adjust the stack
    and save or restore the return address.  Due to grouping multiple
    save/restore stub functions together the save/restore 0 calls actually
    save s0, s1, s2, and the return address, but only the return address
    actually matters.  Leaf functions don't call the save/restore stubs,
    so whenever we do see a call to the save/restore stubs, the store of
    the return address is required.
    
    If we look in gcc/config/riscv/riscv.c at the function
    riscv_expand_prologue and riscv_expand_epilogue we can see that it
    would be reasonably easy to adjust these functions to avoid the calls
    to the save/restore stubs for those cases where we are about to call
    _riscv_save_0 and _riscv_restore_0, however, the actual code size
    saving this would give is debatable, with linker relaxation, the calls
    to save/restore are often just 4-bytes, and can sometimes even be
    2-bytes, while leaving the stack adjust and return address save inline
    is always going to be 4-bytes.
    
    The interesting case is when we call _riscv_save_0 and
    _riscv_restore_0, and also have a frame that would (without
    save/restore) have resulted in a tail call.  In this case if we could
    remove the save/restore calls, and restore the tail call then we would
    get a real size saving.
    
    The problem is that the choice of generating a tail call or not is
    done during the gimple expand pass, at which point we don't know how
    many registers we need to save (or restore).
    
    The solution presented in this patch offers a partial solution to this
    problem.  By using the TARGET_MACHINE_DEPENDENT_REORG pass to
    implement a very limited pattern matching we identify functions that
    call _riscv_save_0 and _riscv_restore_0, and which could be converted
    to make use of a tail call.  These functions are then converted to the
    non save/restore tail call form.
    
    This should result in a code size reduction when compiling with -Os
    and with the -msave-restore flag.
    
    gcc/ChangeLog:
    
            * config.gcc: Add riscv-sr.o to extra_objs for riscv.
            * config/riscv/riscv-sr.c: New file.
            * config/riscv/riscv.c (riscv_reorg): New function.
            (TARGET_MACHINE_DEPENDENT_REORG): Define.
            * config/riscv/riscv.h (SIBCALL_REG_P): Define.
            (riscv_remove_unneeded_save_restore_calls): Declare.
            * config/riscv/t-riscv (riscv-sr.o): New build rule.
    
    gcc/testsuite/ChangeLog:
    
            * gcc.target/riscv/save-restore-2.c: New file.
            * gcc.target/riscv/save-restore-3.c: New file.
            * gcc.target/riscv/save-restore-4.c: New file.
            * gcc.target/riscv/save-restore-5.c: New file.
            * gcc.target/riscv/save-restore-6.c: New file.
            * gcc.target/riscv/save-restore-7.c: New file.
            * gcc.target/riscv/save-restore-8.c: New file.

diff --git a/gcc/config.gcc b/gcc/config.gcc
index bdc2253f8ef..b4440877e99 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -523,7 +523,7 @@ pru-*-*)
 	;;
 riscv*)
 	cpu_type=riscv
-	extra_objs="riscv-builtins.o riscv-c.o"
+	extra_objs="riscv-builtins.o riscv-c.o riscv-sr.o"
 	d_target_objs="riscv-d.o"
 	;;
 rs6000*-*-*)
diff --git a/gcc/config/riscv/riscv-sr.c b/gcc/config/riscv/riscv-sr.c
new file mode 100644
index 00000000000..977f21b3e37
--- /dev/null
+++ b/gcc/config/riscv/riscv-sr.c
@@ -0,0 +1,465 @@
+/* This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+/* This file contains code aimed at optimizing function generated with the
+   use of '-msave-restore.  The goal is to identify cases where the call
+   out to the save/restore routines are sub-optimal, and remove the calls
+   in this case.
+
+   As GCC currently makes the choice between using or not using
+   save/restore early on (during the gimple expand pass) once we have
+   selected to use save/restore we are stuck with it.  */
+
+#define IN_TARGET_CODE 1
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tm.h"
+#include "rtl.h"
+#include "function.h"
+#include "memmodel.h"
+#include "emit-rtl.h"
+#include "target.h"
+#include "basic-block.h"
+#include "bitmap.h"
+#include "df.h"
+#include "tree.h"
+#include "expr.h"
+#include "cfg.h"
+
+/* This file should be included last.  */
+#include "hard-reg-set.h"
+
+/* Look in the function prologue for a call to the save stub.  Ensure that
+   the instruction is as we expect (see detail below) and if the
+   instruction matches return a pointer to it.  Otherwise, return NULL.
+
+   We expect the function prologue to look like this:
+
+   (note NOTE_INSN_BASIC_BLOCK)
+   (insn (parallel [
+	          (unspec_volatile [
+	                  (const_int 2 [0x2])
+	              ] UNSPECV_GPR_SAVE)
+	          (clobber (reg:SI 5 t0))
+	          (clobber (reg:SI 6 t1))])
+   (note NOTE_INSN_PROLOGUE_END)
+
+   Between the NOTE_INSN_BASIC_BLOCK and the GPR_SAVE insn we might find
+   other notes of type NOTE_INSN_DELETED and/or NOTE_INSN_FUNCTION_BEG.
+
+   The parameter BODY is updated to point to the first instruction after
+   the NOTE_INSN_PROLOGUE_END or will be updated to NULL if the prologue
+   end note was not found.  */
+
+static rtx_insn *
+riscv_sr_match_prologue (rtx_insn **body)
+{
+  rtx_insn *insn, *bb_note;
+  *body = NULL;
+
+  /* Find the prologue end note.  */
+  for (insn = get_insns (); insn != NULL; insn = NEXT_INSN (insn))
+    if (NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_PROLOGUE_END)
+      {
+	*body = NEXT_INSN (insn);
+	break;
+      }
+
+  /* If we don't have the prologue end note and at least one instruction
+     before it, then this function doesn't have the structure we expect.  */
+  if (insn == NULL
+      || PREV_INSN (insn) == NULL)
+    return NULL;
+
+  /* The INSN is the end of prologue note, before this we expect to find
+     one real instruction which makes the prologue, and before that we
+     expect to find some number of notes for deleted instructions, the
+     beginning of the function, and finally a basicblock beginning.  The
+     following loop checks that this assumption is true.  */
+  for (bb_note = PREV_INSN (PREV_INSN (insn));
+       bb_note != NULL;
+       bb_note = PREV_INSN (bb_note))
+    {
+      if (!NOTE_P (bb_note))
+	return NULL;
+      if (NOTE_KIND (bb_note) == NOTE_INSN_BASIC_BLOCK)
+	break;
+      if (NOTE_KIND (bb_note) != NOTE_INSN_DELETED
+	  && NOTE_KIND (bb_note) != NOTE_INSN_FUNCTION_BEG)
+	return NULL;
+    }
+  if (bb_note == NULL)
+    return NULL;
+
+  /* Set INSN to point to the actual interesting prologue instruction.  */
+  insn = PREV_INSN (insn);
+  if (INSN_P (insn)
+      && INSN_CODE (insn) == CODE_FOR_gpr_save
+      /* Check this is a call to _riscv_save_0.  */
+      && GET_CODE (PATTERN (insn)) == PARALLEL
+      && GET_CODE (XVECEXP (PATTERN (insn), 0, 0)) == UNSPEC_VOLATILE
+      && (GET_CODE (XVECEXP (XVECEXP (PATTERN (insn), 0, 0), 0, 0))
+	  == CONST_INT)
+      && XINT (XVECEXP (XVECEXP (PATTERN (insn), 0, 0), 0, 0), 0) == 2)
+    return insn;
+
+  return NULL;
+}
+
+/* Find the first instruction in the epilogue of the current function, and
+   return a pointer to that instruction if, and only if, the epilogue has
+   the correct structure that would allow us to optimize out the call to
+   _riscv_restore_0.  */
+
+static rtx_insn *
+riscv_sr_match_epilogue (void)
+{
+  /* Find the first instruction in the epilogue.  */
+  rtx_insn *insn, *start;
+  for (insn = get_insns (); insn != NULL; insn = NEXT_INSN (insn))
+    if (NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_EPILOGUE_BEG)
+      {
+	insn = NEXT_INSN (insn);
+	break;
+      }
+  if (insn == NULL)
+    return NULL;
+
+  /* At this point INSN is the first instruction in the epilogue.  A
+     standard epilogue (of the form we expect to handle) consists of the
+     following instructions:
+
+     1. A stack_tiesi or stack_tiedi (for RV32 and RV64 respectively),
+
+     2. An optional use instruction for the register holding the return
+        value.  This will be missing in functions with no return value,
+
+     3. A gpr_restore instruction, and
+
+     4. A jump instruction of type gpr_restore_return.  */
+  start = insn;
+  if (INSN_CODE (insn) != CODE_FOR_stack_tiesi
+      && INSN_CODE (insn) != CODE_FOR_stack_tiedi)
+    return NULL;
+
+  insn = NEXT_INSN (insn);
+  if (INSN_P (insn) && GET_CODE (PATTERN (insn)) == USE)
+    insn = NEXT_INSN (insn);
+
+  if (!INSN_P (insn) || INSN_CODE (insn) != CODE_FOR_gpr_restore)
+    return NULL;
+
+  insn = NEXT_INSN (insn);
+  if (!INSN_P (insn) || INSN_CODE (insn) != CODE_FOR_gpr_restore_return)
+    return NULL;
+
+  return start;
+}
+
+/* Helper for riscv_remove_unneeded_save_restore_calls.  If we match the
+   prologue instructions but not the epilogue then we might have the case
+   where the epilogue has been optimised out due to a call to a no-return
+   function.  In this case we might be able to remove the prologue too -
+   that's what this function does.  PROLOGUE is the matched prolgoue
+   instruction, by the time this function returns the progloue instruction
+   may have been removed.  */
+
+static void
+check_for_no_return_call (rtx_insn *prologue)
+{
+  /* Check to see if we have the following pattern:
+
+     PROLOGUE instruction
+     NOTE_INSN_PROLOGUE_END
+     A no-return call instruction
+
+     If we do, then we can remove the prologue instruction safely. Remember
+     that we've already confirmed by this point that the prologue is a call
+     to riscv_save_0.  */
+
+  if (dump_file)
+    fprintf (dump_file,
+	     "Prologue matched, checking for no-return epilogue.\n");
+
+  rtx_insn *tmp = NEXT_INSN (prologue);
+  if (!NOTE_P (tmp) || NOTE_KIND (tmp) != NOTE_INSN_PROLOGUE_END)
+    return;
+
+  /* Skip any extra notes in here, they're most likely just debug.  */
+  do
+    {
+      tmp = NEXT_INSN (tmp);
+    }
+  while (tmp != NULL && NOTE_P (tmp));
+
+  if (tmp == NULL || !INSN_P (tmp))
+    return;
+
+  bool noreturn_p = find_reg_note (tmp, REG_NORETURN, NULL_RTX) != NULL_RTX;
+  if (!CALL_P (tmp) || !noreturn_p)
+    return;
+
+  if (dump_file)
+    fprintf (dump_file,
+	     "Prologue call to riscv_save_0 followed by noreturn call, "
+	     "removing prologue.\n");
+  remove_insn (prologue);
+}
+
+/* Entry point called from riscv_reorg to remove some unneeded calls to
+   the save and restore stubs.  This should only be called when
+   -msave-restore is in use.
+
+   We identify some simple cases where the function looks like this:
+
+   call t0,__riscv_save_0
+   <other-code>
+   call foo
+   tail __riscv_restore_0
+
+   And transform it into something like this:
+
+   <other-code>
+   tail foo
+
+   In the above examples, what can appear in <other-code> is pretty
+   restricted; only caller saved registers can be touched, this prevents
+   any additional calls (as they would write to 'ra').  */
+
+void
+riscv_remove_unneeded_save_restore_calls (void)
+{
+  /* Will point to the first instruction of the function body, after the
+     prologue end note.  */
+  rtx_insn *body = NULL;
+
+  /* Should only be called with -msave-restore is in use.  */
+  gcc_assert (TARGET_SAVE_RESTORE);
+
+  /* Match the expected prologue and epilogue patterns.  If either of these
+     fail to match then we abandon our attempt to optimize this function.  */
+  rtx_insn *prologue_matched = riscv_sr_match_prologue (&body);
+  if (prologue_matched == NULL || body == NULL)
+    return;
+
+  rtx_insn *epilogue_matched = riscv_sr_match_epilogue ();
+  if (epilogue_matched == NULL)
+    {
+      check_for_no_return_call (prologue_matched);
+      return;
+    }
+
+  if (dump_file)
+    fprintf (dump_file,
+	     "Could be a candidate for save/restore removal\n");
+
+  /* We want to check which registers this function uses.  */
+  df_analyze ();
+
+  int call_count = 0;
+  bool good_use = true;
+  int epilogue_count = 0;
+
+  /* Now examine all of the instructions that make up this function, we're
+     looking for call instructions and also double checking register usage
+     while we're at it (see comments below).  */
+  basic_block bb;
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      rtx_insn *insn;
+
+      FOR_BB_INSNS (bb, insn)
+	{
+	  if (dump_file)
+	    fprintf (dump_file,
+		     "Block %d, Insn %d\n", bb->index, INSN_UID (insn));
+
+	  /* If we scan the epilogue we will fall foul of our register
+	     useage check below (due to it's use of the return address), so
+	     once we spot we're at the epilogue, just skip the rest of this
+	     block.  Scanning the prologue instructions again (if they
+	     match the expected pattern) is harmless.  */
+	  if (NOTE_P (insn)
+	      && NOTE_KIND (insn) == NOTE_INSN_EPILOGUE_BEG)
+	    {
+	      ++epilogue_count;
+	      break;
+	    }
+
+	  if (!INSN_P (insn))
+	    continue;
+
+	  if (CALL_P (insn))
+	    ++call_count;
+	  else
+	    {
+	      df_ref use;
+
+	      FOR_EACH_INSN_USE (use, insn)
+		{
+		  /* If the function makes use of any registers that are
+		     callee saved then we should be saving them in this
+		     function, which would suggest that a call to the save
+		     and restore functions is required.  This would seem to
+		     indicate that something has gone wrong above, as we
+		     should only get here if we are saving zero registers.
+
+		     The one exception to this rule is the return address
+		     register used within a call instruction.  We can
+		     optimize a single call within a function (by making it
+		     a tail call), so we skip call instructions here.  */
+		  if (!call_used_regs[DF_REF_REGNO (use)])
+		    {
+		      if (dump_file)
+			fprintf (dump_file,
+				 "Found unsupported use of callee saved "
+				 "register in instruction %d\n",
+				 INSN_UID (insn));
+		      good_use = false;
+		      break;
+		    }
+		}
+	      if (!good_use)
+		break;
+	    }
+	}
+    }
+
+  /* If we used any registers that would indicate a need for a call to a
+     save/restore stub then don't optimize.  */
+  if (!good_use)
+    return;
+
+  /* If this function has multiple epilogues, then for now we don't try to
+     optimise it.  */
+  if (epilogue_count != 1)
+    return;
+
+  /* We can only optimize functions containing a single call, any more
+     would require us to add instructions to store the return address on
+     the stack (and restore it before we return).  We could do this in the
+     future, but for now we don't.  A single call can be transformed into
+     a tail call reasonably easily.  */
+  if (call_count > 1)
+    {
+      if (dump_file)
+	fprintf (dump_file,
+		 "Found too many call instructions\n");
+      return;
+    }
+
+  rtx_insn *epilogue_begin_note = PREV_INSN (epilogue_matched);
+  gcc_assert (NOTE_P (epilogue_begin_note)
+	      && NOTE_KIND (epilogue_begin_note) == NOTE_INSN_EPILOGUE_BEG);
+
+  df_finish_pass (false);
+
+  /* Find the first instruction before the function epilogue.  */
+  rtx_insn *insn_before_epilogue;
+  for (insn_before_epilogue = PREV_INSN (epilogue_begin_note);
+       NOTE_P (insn_before_epilogue);
+       insn_before_epilogue = PREV_INSN (insn_before_epilogue))
+    ;
+
+  /* Leaf functions will not generate calls to the save/restore stubs, so
+     there's no need for this optimization there.  We know this function
+     has no more than 1 call (checked above).  To convert this single call
+     into a tail call we rely on the call being the last thing before the
+     epilogue.  */
+  if (GET_CODE (insn_before_epilogue) != CALL_INSN)
+    return;
+
+  /* The last instruction in this block, just before the epilogue is a
+     call.  We can potentially change this call into a tail call.  */
+  rtx_insn *call = insn_before_epilogue;
+
+  /* Transform call in insn to a sibcall, this will only be done if the
+     last thing in the function is a call.  */
+  rtx callpat = PATTERN (call);
+  gcc_assert (GET_CODE (callpat) == PARALLEL);
+
+  /* Extract from CALLPAT the information we need to build the sibcall.  */
+  rtx target_call = NULL;
+  rtx tmp_rtx = XVECEXP (callpat, 0, 0);
+  rtx set_target = NULL;
+  switch (GET_CODE (tmp_rtx))
+    {
+    case CALL:
+      target_call = tmp_rtx;
+      break;
+
+    case SET:
+      {
+	set_target = XEXP (tmp_rtx, 0);
+	tmp_rtx = XEXP (tmp_rtx, 1);
+	if (GET_CODE (tmp_rtx) != CALL)
+	  return;
+	target_call = tmp_rtx;
+	break;
+      }
+
+    default:
+      return;
+    }
+
+  rtx target_mem = XEXP (target_call, 0);
+  if (GET_CODE (target_mem) != MEM)
+    return;
+
+  rtx target = XEXP (target_mem, 0);
+  if (GET_CODE (target) != SYMBOL_REF && GET_CODE (target) != REG)
+    return;
+
+  /* The sibcall instructions can only use a specific subset of
+     registers, we're about to (possibly) move a call through a
+     register from the function body and make it a sibcall.  If we're
+     not using an appropriate register then we can't make this change.
+
+     Maybe in some future iteration we could actually scan the
+     function, find a suitable sibcall register, and switch over the
+     registers.  But we don't do that yet.  */
+  if (GET_CODE (target) == REG
+      && !SIBCALL_REG_P (REGNO (target)))
+    return;
+
+  rtx sibcall = NULL;
+  if (set_target != NULL)
+    sibcall
+      = gen_sibcall_value_internal (set_target, target, const0_rtx);
+  else
+    sibcall = gen_sibcall_internal (target, const0_rtx);
+
+  rtx_insn *before_call = PREV_INSN (call);
+  remove_insn (call);
+  rtx_insn *insn = emit_call_insn_after_setloc (sibcall, before_call,
+						INSN_LOCATION (call));
+  REG_NOTES (insn) = REG_NOTES (call);
+  SIBLING_CALL_P (insn) = 1;
+
+  /* Now update the prologue and epilogue to take account of the
+     changes within the function body.  */
+  remove_insn (prologue_matched);
+  remove_insn (NEXT_INSN (NEXT_INSN (NEXT_INSN (epilogue_matched))));
+  remove_insn (NEXT_INSN (NEXT_INSN (epilogue_matched)));
+  remove_insn (NEXT_INSN (epilogue_matched));
+  remove_insn (epilogue_matched);
+
+  if (dump_file)
+    fprintf (dump_file,
+	     "Save/restore successfully removed\n");
+}
diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
index 77a3ad94aa8..11a43c1b64d 100644
--- a/gcc/config/riscv/riscv.c
+++ b/gcc/config/riscv/riscv.c
@@ -5007,6 +5007,16 @@ riscv_promote_function_mode (const_tree type ATTRIBUTE_UNUSED,
   return mode;
 }
 
+/* Implement TARGET_MACHINE_DEPENDENT_REORG.  */
+
+static void
+riscv_reorg (void)
+{
+  /* Do nothing unless we have -msave-restore */
+  if (TARGET_SAVE_RESTORE)
+    riscv_remove_unneeded_save_restore_calls ();
+}
+
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP
 #define TARGET_ASM_ALIGNED_HI_OP "\t.half\t"
@@ -5181,6 +5191,9 @@ riscv_promote_function_mode (const_tree type ATTRIBUTE_UNUSED,
 #undef TARGET_CUSTOM_FUNCTION_DESCRIPTORS
 #define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
 
+#undef TARGET_MACHINE_DEPENDENT_REORG
+#define TARGET_MACHINE_DEPENDENT_REORG riscv_reorg
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-riscv.h"
diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
index 246494663f6..b1e3403f8a7 100644
--- a/gcc/config/riscv/riscv.h
+++ b/gcc/config/riscv/riscv.h
@@ -289,6 +289,10 @@ along with GCC; see the file COPYING3.  If not see
 #define FP_REG_P(REGNO)  \
   ((unsigned int) ((int) (REGNO) - FP_REG_FIRST) < FP_REG_NUM)
 
+/* True when REGNO is in SIBCALL_REGS set.  */
+#define SIBCALL_REG_P(REGNO)	\
+  TEST_HARD_REG_BIT (reg_class_contents[SIBCALL_REGS], REGNO)
+
 #define FP_REG_RTX_P(X) (REG_P (X) && FP_REG_P (REGNO (X)))
 
 /* Use s0 as the frame pointer if it is so requested.  */
@@ -918,4 +922,8 @@ extern unsigned riscv_stack_boundary;
 #define SWSP_REACH (4LL << C_SxSP_BITS)
 #define SDSP_REACH (8LL << C_SxSP_BITS)
 
+/* Called from RISCV_REORG, this is defined in riscv-sr.c.  */
+
+extern void riscv_remove_unneeded_save_restore_calls (void);
+
 #endif /* ! GCC_RISCV_H */
diff --git a/gcc/config/riscv/t-riscv b/gcc/config/riscv/t-riscv
index ece3a75d512..5ecb3c160a6 100644
--- a/gcc/config/riscv/t-riscv
+++ b/gcc/config/riscv/t-riscv
@@ -5,6 +5,11 @@ riscv-builtins.o: $(srcdir)/config/riscv/riscv-builtins.c $(CONFIG_H) \
 	$(COMPILER) -c $(ALL_COMPILERFLAGS) $(ALL_CPPFLAGS) $(INCLUDES) \
 		$(srcdir)/config/riscv/riscv-builtins.c
 
+riscv-sr.o: $(srcdir)/config/riscv/riscv-sr.c $(CONFIG_H) \
+  $(SYSTEM_H)
+	$(COMPILER) -c $(ALL_COMPILERFLAGS) $(ALL_CPPFLAGS) $(INCLUDES) \
+		$(srcdir)/config/riscv/riscv-sr.c
+
 riscv-c.o: $(srcdir)/config/riscv/riscv-c.c $(CONFIG_H) $(SYSTEM_H) \
     coretypes.h $(TM_H) $(TREE_H) output.h $(C_COMMON_H) $(TARGET_H)
 	$(COMPILER) -c $(ALL_COMPILERFLAGS) $(ALL_CPPFLAGS) $(INCLUDES) \
diff --git a/gcc/testsuite/gcc.target/riscv/save-restore-2.c b/gcc/testsuite/gcc.target/riscv/save-restore-2.c
new file mode 100644
index 00000000000..204bf67b66e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/save-restore-2.c
@@ -0,0 +1,22 @@
+/* { dg-options "-Os -msave-restore" } */
+
+/* With -msave-restore in use it should not be possible to remove the calls
+   to the save and restore stubs in this case (in current GCC).  */
+
+extern void fn2 ();
+
+volatile int a = 0;
+
+int
+fn1 ()
+{
+  fn2 ();
+
+  while (a)
+    ;
+
+  return 0;
+}
+
+/* { dg-final { scan-assembler "call\[ \t\]*t0,__riscv_save_0" } } */
+/* { dg-final { scan-assembler "tail\[ \t\]*__riscv_restore_0" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/save-restore-3.c b/gcc/testsuite/gcc.target/riscv/save-restore-3.c
new file mode 100644
index 00000000000..6bf9fb014d6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/save-restore-3.c
@@ -0,0 +1,16 @@
+/* { dg-options "-Os -msave-restore" } */
+
+/* With -msave-restore in use GCC should be able to remove the calls to the
+   save and restore stubs in this case, replacing them with a tail call to
+   foo.  */
+
+extern int foo ();
+
+int bar ()
+{
+  return foo ();
+}
+
+/* { dg-final { scan-assembler-not "call\[ \t\]*t0,__riscv_save_0" } } */
+/* { dg-final { scan-assembler-not "tail\[ \t\]*__riscv_restore_0" } } */
+/* { dg-final { scan-assembler "tail\[ \t\]*foo" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/save-restore-4.c b/gcc/testsuite/gcc.target/riscv/save-restore-4.c
new file mode 100644
index 00000000000..c1c10c604a1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/save-restore-4.c
@@ -0,0 +1,27 @@
+/* { dg-options "-Os -msave-restore" } */
+
+/* This test covers a case where we can't (currently) remove the calls to
+   the save/restore stubs.  The cast of the return value from BAR requires
+   a sign extension between the call to BAR, and the return from FOO, this
+   currently prevents the removal of the save/restore calls.  */
+
+typedef enum SomeType
+  {
+   A = 0,
+   B = 1,
+   C = 2,
+   D = 3
+  }
+  SomeType;
+
+typedef unsigned int u_32;
+
+extern u_32 bar (u_32 arg);
+
+SomeType foo (u_32 arg)
+{
+  return (SomeType) bar (arg);
+}
+
+/* { dg-final { scan-assembler "call\[ \t\]*t0,__riscv_save_0" } } */
+/* { dg-final { scan-assembler "tail\[ \t\]*__riscv_restore_0" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/save-restore-5.c b/gcc/testsuite/gcc.target/riscv/save-restore-5.c
new file mode 100644
index 00000000000..fe0ffdcd504
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/save-restore-5.c
@@ -0,0 +1,9 @@
+typedef int (*FPTR) (void);
+FPTR a;
+
+int
+func ()
+{
+  int b = a ();
+  return b;
+}
diff --git a/gcc/testsuite/gcc.target/riscv/save-restore-6.c b/gcc/testsuite/gcc.target/riscv/save-restore-6.c
new file mode 100644
index 00000000000..530865456a2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/save-restore-6.c
@@ -0,0 +1,16 @@
+/* { dg-options "-Os -msave-restore" } */
+
+/* With -msave-restore in use GCC should be able to remove the calls to the
+   save and restore stubs in this case, replacing them with a tail call to
+   other_func.  */
+
+extern void other_func ();
+
+void func ()
+{
+  other_func ();
+}
+
+/* { dg-final { scan-assembler-not "call\[ \t\]*t0,__riscv_save_0" } } */
+/* { dg-final { scan-assembler-not "tail\[ \t\]*__riscv_restore_0" } } */
+/* { dg-final { scan-assembler "tail\[ \t\]*other_func" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/save-restore-7.c b/gcc/testsuite/gcc.target/riscv/save-restore-7.c
new file mode 100644
index 00000000000..06719c4e413
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/save-restore-7.c
@@ -0,0 +1,30 @@
+/* { dg-options "-Os -msave-restore" } */
+
+/* With -msave-restore in use it should not be possible to remove the calls
+   to the save and restore stubs in this case (in current GCC).  */
+
+enum
+  {
+   VAL_A,
+   VAL_B,
+   VAL_C,
+   VAL_D
+  } a;
+
+extern void other_1 ();
+extern void other_2 ();
+
+void func ()
+{
+  switch (a)
+    {
+    case VAL_B:
+    case VAL_C:
+      other_1 ();
+    case VAL_D:
+      other_2 ();
+    }
+}
+
+/* { dg-final { scan-assembler "call\[ \t\]*t0,__riscv_save_0" } } */
+/* { dg-final { scan-assembler "tail\[ \t\]*__riscv_restore_0" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/save-restore-8.c b/gcc/testsuite/gcc.target/riscv/save-restore-8.c
new file mode 100644
index 00000000000..8880cd288ee
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/save-restore-8.c
@@ -0,0 +1,12 @@
+/* { dg-options "-Os -msave-restore" } */
+
+/* As a leaf function this should never have the calls to the save and
+   restore stubs added, but lets check anyway.  */
+
+int func ()
+{
+  return 3;
+}
+
+/* { dg-final { scan-assembler-not "call\[ \t\]*t0,__riscv_save_0" } } */
+/* { dg-final { scan-assembler-not "tail\[ \t\]*__riscv_restore_0" } } */

On Mon, Oct 21, 2019 at 5:26 AM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> Below is a new versions of this patch, I believe that this addresses
> the review comments from the earlier version.  In addition this has
> been tested using Jim's idea of forcing -msave-restore (if the flag is
> not otherwise given) and I now see no test failures for newlib and
> linux toolchains.
>
> One thing that has been mentioned briefly was adding a flag to control
> just this optimisation, I haven't done that yet, but can if that is
> required - obviously this optimisation doesn't do anything if
> -msave-restore is turned off, so that is always an option.  With no
> test failures I don't know if a separate flag is required.

I can live without the flag to control it, since as you mention
-msave-restore will turn it off.

I'm seeing failures with the new save-restore-4.c testcase for 32-bit
targets.  It works for 64-bit but not for 32-bit, because the
sign-extension it is checking for only happens for 64-bit targets.
The testcase should check for a target of riscv64*-*-* or else hard
code -march=rv64gc etc options.  Either fix seems reasonable.

You fixed some British spellings of optimise to optimize, but there
are two new comments that add two more uses of optimise which is
inconsistent.

I also noticed a "useage" that should be "usage".

Otherwise this looks good to me.  It is OK to check in with the above
minor issues fixed.

Jim

* Jim Wilson <jimw@sifive.com> [2019-10-22 16:34:53 -0700]:

> On Mon, Oct 21, 2019 at 5:26 AM Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > Below is a new versions of this patch, I believe that this addresses
> > the review comments from the earlier version.  In addition this has
> > been tested using Jim's idea of forcing -msave-restore (if the flag is
> > not otherwise given) and I now see no test failures for newlib and
> > linux toolchains.
> >
> > One thing that has been mentioned briefly was adding a flag to control
> > just this optimisation, I haven't done that yet, but can if that is
> > required - obviously this optimisation doesn't do anything if
> > -msave-restore is turned off, so that is always an option.  With no
> > test failures I don't know if a separate flag is required.
> 
> I can live without the flag to control it, since as you mention
> -msave-restore will turn it off.
> 
> I'm seeing failures with the new save-restore-4.c testcase for 32-bit
> targets.  It works for 64-bit but not for 32-bit, because the
> sign-extension it is checking for only happens for 64-bit targets.
> The testcase should check for a target of riscv64*-*-* or else hard
> code -march=rv64gc etc options.  Either fix seems reasonable.
> 
> You fixed some British spellings of optimise to optimize, but there
> are two new comments that add two more uses of optimise which is
> inconsistent.
> 
> I also noticed a "useage" that should be "usage".
> 
> Otherwise this looks good to me.  It is OK to check in with the above
> minor issues fixed.

Jim,

Thanks for all your help reviewing this patch.

I've now merged this with fixes for the issues you identified above.
Let me know if you run into any problems.

Thanks,
Andrew