RISC-V: Include more registers in SIBCALL_REGS.

This finishes the part 1 of 2 patch submitted by Andrew Burgess on Aug 19.
This adds the argument registers but not t0 (aka x5) to SIBCALL_REGS.  It
also adds the missing riscv_regno_to_class change.

Tested with cross riscv32-elf and riscv64-linux toolchain build and check.
There were no regressions.  I see about a 0.01% code size reduction for the
C and libstdc++ libraries.

Committed.

Jim

	gcc/
	* config/riscv/riscv.h (REG_CLASS_CONTENTS): Add argument passing
	regs to SIBCALL_REGS.
	* config/riscv/riscv.c (riscv_regno_to_class): Change argument
	passing regs to SIBCALL_REGS.
---
 gcc/config/riscv/riscv.c | 6 +++---
 gcc/config/riscv/riscv.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

* Jim Wilson <jimw@sifive.com> [2019-10-16 14:04:45 -0700]:

> This finishes the part 1 of 2 patch submitted by Andrew Burgess on Aug 19.
> This adds the argument registers but not t0 (aka x5) to SIBCALL_REGS.  It
> also adds the missing riscv_regno_to_class change.
> 
> Tested with cross riscv32-elf and riscv64-linux toolchain build and check.
> There were no regressions.  I see about a 0.01% code size reduction for the
> C and libstdc++ libraries.
> 
> Committed.

Thanks for doing this Jim.

I'm still working on part 2, I'm hoping to have a revised patch posted
by Monday next week.

Thanks again,
Andrew



> 
> Jim
> 
> 	gcc/
> 	* config/riscv/riscv.h (REG_CLASS_CONTENTS): Add argument passing
> 	regs to SIBCALL_REGS.
> 	* config/riscv/riscv.c (riscv_regno_to_class): Change argument
> 	passing regs to SIBCALL_REGS.
> ---
>  gcc/config/riscv/riscv.c | 6 +++---
>  gcc/config/riscv/riscv.h | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
> index b8a8778b92c..77a3ad94aa8 100644
> --- a/gcc/config/riscv/riscv.c
> +++ b/gcc/config/riscv/riscv.c
> @@ -256,9 +256,9 @@ enum riscv_microarchitecture_type riscv_microarchitecture;
>  const enum reg_class riscv_regno_to_class[FIRST_PSEUDO_REGISTER] = {
>    GR_REGS,	GR_REGS,	GR_REGS,	GR_REGS,
>    GR_REGS,	GR_REGS,	SIBCALL_REGS,	SIBCALL_REGS,
> -  JALR_REGS,	JALR_REGS,	JALR_REGS,	JALR_REGS,
> -  JALR_REGS,	JALR_REGS,	JALR_REGS,	JALR_REGS,
> -  JALR_REGS,	JALR_REGS, 	JALR_REGS,	JALR_REGS,
> +  JALR_REGS,	JALR_REGS,	SIBCALL_REGS,	SIBCALL_REGS,
> +  SIBCALL_REGS,	SIBCALL_REGS,	SIBCALL_REGS,	SIBCALL_REGS,
> +  SIBCALL_REGS,	SIBCALL_REGS,	JALR_REGS,	JALR_REGS,
>    JALR_REGS,	JALR_REGS,	JALR_REGS,	JALR_REGS,
>    JALR_REGS,	JALR_REGS,	JALR_REGS,	JALR_REGS,
>    SIBCALL_REGS,	SIBCALL_REGS,	SIBCALL_REGS,	SIBCALL_REGS,
> diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
> index 5fc9be8edbf..246494663f6 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 */	\
> +  { 0xf003fcc0, 0x00000000, 0x00000000 },	/* SIBCALL_REGS */	\
>    { 0xffffffc0, 0x00000000, 0x00000000 },	/* JALR_REGS */		\
>    { 0xffffffff, 0x00000000, 0x00000000 },	/* GR_REGS */		\
>    { 0x00000000, 0xffffffff, 0x00000000 },	/* FP_REGS */		\
> -- 
> 2.17.1
>

On Thu, Oct 17, 2019 at 7:09 AM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> I'm still working on part 2, I'm hoping to have a revised patch posted
> by Monday next week.

I started looking at the part 2 patch also.  I noticed a problem where
the NOTE_INSN_EPILOGUE_BEGIN can occur before the
NOTE_INSN_PROLOGUE_END due to branch and basic block optimizations, I
get an ICE in this case due to deref of a null pointer.  This is
pretty easy to fix, I just added a check to
riscv_remove_unneeded_save_restore_calls in the loop that searches for
NOTE_INSN_PROLOGUE_END, and if I find NOTE_INSN_EPILOGUE_BEGIN first I
exit without doing anything.  A more complex fix would be to try to
follow the CFG instead of scanning forward from the prologue end note
to find the epilogue begin note.  I also noticed a case where the
__riscv_save_0 call was optimized away but the __riscv_restore_0 call
was not.  In this case the function has two epilogues, and only one of
the two epilogues was optimized.  We could just check for more than
one epilogue and return without doing any work as the simple fix.  Or
a more complex fix is to try to handle more than one epilogue.

Given the kinds of problems I'm seeing, I think there should be an
option to control this optimization, so people can turn it off is
necessary.  I think it is OK if this is on by default if it passes the
gcc testsuite.

Since you are looking at this, I can look at something else for a few
days.  I was just trying to get this off of my backlog.

Jim

* Jim Wilson <jimw@sifive.com> [2019-10-17 14:55:34 -0700]:

> On Thu, Oct 17, 2019 at 7:09 AM Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > I'm still working on part 2, I'm hoping to have a revised patch posted
> > by Monday next week.
> 
> I started looking at the part 2 patch also.  I noticed a problem where
> the NOTE_INSN_EPILOGUE_BEGIN can occur before the
> NOTE_INSN_PROLOGUE_END due to branch and basic block optimizations, I
> get an ICE in this case due to deref of a null pointer.  This is
> pretty easy to fix, I just added a check to
> riscv_remove_unneeded_save_restore_calls in the loop that searches for
> NOTE_INSN_PROLOGUE_END, and if I find NOTE_INSN_EPILOGUE_BEGIN first I
> exit without doing anything.  A more complex fix would be to try to
> follow the CFG instead of scanning forward from the prologue end note
> to find the epilogue begin note.  I also noticed a case where the
> __riscv_save_0 call was optimized away but the __riscv_restore_0 call
> was not.  In this case the function has two epilogues, and only one of
> the two epilogues was optimized.  We could just check for more than
> one epilogue and return without doing any work as the simple fix.  Or
> a more complex fix is to try to handle more than one epilogue.
> 
> Given the kinds of problems I'm seeing, I think there should be an
> option to control this optimization, so people can turn it off is
> necessary.  I think it is OK if this is on by default if it passes the
> gcc testsuite.
> 
> Since you are looking at this, I can look at something else for a few
> days.  I was just trying to get this off of my backlog.

It's entirely up to you.  I believe I have a version testing now that
addresses all of the issues you mentioned above.  I haven't added a
switch for it, but can do if you want.

My expectation was that I would post a version on Monday that had zero
regressions when run with -msave-restore forced on (as you described
in a previous mail).

I'll continue to work on this and post on Monday unless you drop a
revision earlier.

Thanks,

Andrew