Re: [PATCH 2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
* Jim Wilson [2019-10-22 16:34:53 -0700]: > On Mon, Oct 21, 2019 at 5:26 AM Andrew Burgess > 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
Re: [PATCH 2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
On Mon, Oct 21, 2019 at 5:26 AM Andrew Burgess 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
Re: [PATCH 2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
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 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 000..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
Re: [PATCH 2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
On Sat, Aug 31, 2019 at 1:08 AM Andreas Schwab 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
Re: [PATCH 2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
On Aug 30 2019, Jim Wilson 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. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Re: [PATCH 2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
On Aug 30 2019, Jim Wilson 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. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Re: [PATCH 2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
On Sun, Aug 25, 2019 at 9:40 AM Jim Wilson 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
Re: [PATCH 2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
On Thu, Aug 22, 2019 at 10:59 PM Jim Wilson 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
Re: [PATCH 2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
On Mon, Aug 19, 2019 at 12:15 PM Andrew Burgess 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
Re: [PATCH 2/2] gcc/riscv: Add a mechanism to remove some calls to _riscv_save_0
On Mon, Aug 19, 2019 at 12:15 PM Andrew Burgess 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
[PATCH 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 diff --git a/gcc/config.gcc b/gcc/config.gcc index 40cbc52dc994..ac02d1835372 100644 --- a/gcc/config.gcc +++ b/gcc/config.gcc @@ -520,7 +520,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 ..7d12c0d82111 --- /dev/null +++ b/gcc/config/riscv/riscv-sr.c @@ -0,0 +1,375 @@ +/* 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
