[1/2] gcc/riscv: Include more registers in SIBCALL_REGS
diff mbox series

Message ID b8da34c1fe62f15164194ba5613e4533c30d300a.1566241796.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • RISCV: Reduce code size when compiling with -msave-restore
Related show

Commit Message

Andrew Burgess Aug. 19, 2019, 7:15 p.m. UTC
The current SIBCALL_REGS are x6, x7, and x28 to x31.  These are all
caller saved registers, however, they are not all of the caller saved
registers.

I don't see any reason why we couldn't add t1, and a0 to a7 into this
set, and this is what this patch does.

gcc/ChangeLog:

	* config/riscv/riscv.h (REG_CLASS_CONTENTS): Update SIBCALL_REGS.
---
 gcc/ChangeLog            | 4 ++++
 gcc/config/riscv/riscv.h | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Andrew Waterman Aug. 19, 2019, 7:31 p.m. UTC | #1
x5 is used as an alternate link register, so using it for sibcalls
will confuse hardware return-address stacks and reduce performance for
implementations that have one.

The reason I excluded a0-a7 is that those are used to pass arguments
to the sibcallee.  It's of course possible that's more restrictive
than necessary, but make sure to test before merging.


On Mon, Aug 19, 2019 at 12:16 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
>
> The current SIBCALL_REGS are x6, x7, and x28 to x31.  These are all
> caller saved registers, however, they are not all of the caller saved
> registers.
>
> I don't see any reason why we couldn't add t1, and a0 to a7 into this
> set, and this is what this patch does.
>
> gcc/ChangeLog:
>
>         * config/riscv/riscv.h (REG_CLASS_CONTENTS): Update SIBCALL_REGS.
> ---
>  gcc/ChangeLog            | 4 ++++
>  gcc/config/riscv/riscv.h | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
> index 5fc9be8edbf2..bb8240bb849a 100644
> --- a/gcc/config/riscv/riscv.h
> +++ b/gcc/config/riscv/riscv.h
> @@ -400,7 +400,7 @@ enum reg_class
>  #define REG_CLASS_CONTENTS                                             \
>  {                                                                      \
>    { 0x00000000, 0x00000000, 0x00000000 },      /* NO_REGS */           \
> -  { 0xf00000c0, 0x00000000, 0x00000000 },      /* SIBCALL_REGS */      \
> +  { 0xf003fce0, 0x00000000, 0x00000000 },      /* SIBCALL_REGS */      \
>    { 0xffffffc0, 0x00000000, 0x00000000 },      /* JALR_REGS */         \
>    { 0xffffffff, 0x00000000, 0x00000000 },      /* GR_REGS */           \
>    { 0x00000000, 0xffffffff, 0x00000000 },      /* FP_REGS */           \
> --
> 2.14.5
>
Jim Wilson Aug. 23, 2019, 5:30 a.m. UTC | #2
On Mon, Aug 19, 2019 at 12:15 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> I don't see any reason why we couldn't add t1, and a0 to a7 into this
> set, and this is what this patch does.

SIBCALL_REGS already includes t1 and t2.  It is t0 aka x5 that is
missing.  I think this is wrong.  As Andrew mentioned, this will
penalize any target that has a call-stack aware branch predictor.  We
could add a tune flag for that, but it doesn't seem worth the effort.
Adding the other regs a0 to a7 is OK.  They won't be used unless they
are available.  This is OK without the t0/x5 change.

Jim
Jim Wilson Aug. 24, 2019, 12:14 a.m. UTC | #3
On Thu, Aug 22, 2019 at 10:30 PM Jim Wilson <jimw@sifive.com> wrote:
>
> On Mon, Aug 19, 2019 at 12:15 PM Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > I don't see any reason why we couldn't add t1, and a0 to a7 into this
> > set, and this is what this patch does.
>
> SIBCALL_REGS already includes t1 and t2.  It is t0 aka x5 that is
> missing.  I think this is wrong.  As Andrew mentioned, this will
> penalize any target that has a call-stack aware branch predictor.  We
> could add a tune flag for that, but it doesn't seem worth the effort.
> Adding the other regs a0 to a7 is OK.  They won't be used unless they
> are available.  This is OK without the t0/x5 change.

While reviewing the second part of the patch set, I noticed that I
missed riscv_regno_to_class.  A change to SIBCALL_REGS requires a
corresponding change to riscv_regno_class.  SIBCALL_REGS is the
smallest class, so you just need to change the entries for a0 to a7
from JALR_REGS to SIBCALL_REGS.

Jim

Patch
diff mbox series

diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
index 5fc9be8edbf2..bb8240bb849a 100644
--- a/gcc/config/riscv/riscv.h
+++ b/gcc/config/riscv/riscv.h
@@ -400,7 +400,7 @@  enum reg_class
 #define REG_CLASS_CONTENTS						\
 {									\
   { 0x00000000, 0x00000000, 0x00000000 },	/* NO_REGS */		\
-  { 0xf00000c0, 0x00000000, 0x00000000 },	/* SIBCALL_REGS */	\
+  { 0xf003fce0, 0x00000000, 0x00000000 },	/* SIBCALL_REGS */	\
   { 0xffffffc0, 0x00000000, 0x00000000 },	/* JALR_REGS */		\
   { 0xffffffff, 0x00000000, 0x00000000 },	/* GR_REGS */		\
   { 0x00000000, 0xffffffff, 0x00000000 },	/* FP_REGS */		\