| Message ID | B5E67142681B53468FAF6B7C31356562441B01AD@hhmail02.hh.imgtec.org |
|---|---|
| State | New |
| Headers | show |
I'd like to give Catherine chance to review this, I notice a couple of formatting nits in the test case: Robert Suchanek <Robert.Suchanek@imgtec.com> writes: > a/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c > b/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c > new file mode 100644 > index 0000000..877d00c > --- /dev/null > +++ b/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c > @@ -0,0 +1,12 @@ > +/* { dg-options "-funroll-loops" } */ > +int foo; > +int bar; > + > +void __attribute__ ((interrupt)) isr(void) { Newline for function name and whitespace before args > + if (!foo) > + { Double space indent for brace or remove them entirely as it is single statement. > + while (bar & 0xFF30); > + } > +} > +/* { dg-final { scan-assembler-not "\\\$8" } } */ Thanks, Matthew
> -----Original Message----- > From: Matthew Fortune [mailto:Matthew.Fortune@imgtec.com] > Sent: Thursday, August 13, 2015 7:20 AM > To: Robert Suchanek; Moore, Catherine > Cc: gcc-patches@gcc.gnu.org > Subject: RE: [PATCH][MIPS] Fix register renaming in the interrupt handlers > > I'd like to give Catherine chance to review this, I notice a couple of formatting > nits in the test case: Yes, this patch is OK with Matthew's suggested changes. Thanks, Catherine > > Robert Suchanek <Robert.Suchanek@imgtec.com> writes: > > a/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c > > b/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c > > new file mode 100644 > > index 0000000..877d00c > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c > > @@ -0,0 +1,12 @@ > > +/* { dg-options "-funroll-loops" } */ int foo; int bar; > > + > > +void __attribute__ ((interrupt)) isr(void) { > > Newline for function name and whitespace before args > > > + if (!foo) > > + { > > Double space indent for brace or remove them entirely as it is single > statement. > > > + while (bar & 0xFF30); > > + } > > +} > > +/* { dg-final { scan-assembler-not "\\\$8" } } */ > > Thanks, > Matthew > >
Robert Suchanek <Robert.Suchanek@imgtec.com> writes: > Hi, > > It was discovered that with the attached test case compiled with -O2 > -funroll-loops, the regrename pass renamed one of the registers ($2) > to $8 that was not saved by the prologue. > > The attached patch fixes it by defining macro HARD_REGNO_RENAME_OK > that returns zero iff the current function is an interrupt handler and > a register was never live. > > Regression is still in progress. Ok to apply if it passes? > > Regards, > Robert > > gcc/ > * config/mips/mips-protos.h (mips_hard_regno_rename_ok): New prototype. > * config/mips/mips.c (mips_hard_regno_rename_ok): New function. > * config/mips/mips.h (HARD_REGNO_RENAME_OK): New. > > gcc/testsuite/ > * gcc.target/mips/interrupt_handler-bug-1.c: New test. You also need to do the same thing for TARGET_HARD_REGNO_SCRATCH_OK, to stop peephole2 from using unsaved registers as scratch registers. I should dig out my patches to clean up this interface. It's just too brittle to have two macros that say what registers can be used after reload. Thanks, Richard
On Aug 13, 2015, at 12:54 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > >> It was discovered that with the attached test case compiled with -O2 >> -funroll-loops, the regrename pass renamed one of the registers ($2) >> to $8 that was not saved by the prologue. >> >> The attached patch fixes it by defining macro HARD_REGNO_RENAME_OK >> that returns zero iff the current function is an interrupt handler and >> a register was never live. > You also need to do the same thing for TARGET_HARD_REGNO_SCRATCH_OK, > to stop peephole2 from using unsaved registers as scratch registers. > > I should dig out my patches to clean up this interface. It's just > too brittle to have two macros that say what registers can be used > after reload. Gosh, I don’t like the interface of them at all. I have interrupt functions and I want all the goodness out of gcc and I want gcc to just know that it can’t use registers it doesn’t want to save for any reason that the port marked as saved by the calling api of the function. :-( Very few ports (3) define TARGET_HARD_REGNO_SCRATCH_OK and many port (19 or so) don’t define this but do seem to have interrupt functions. The existing documentation seems not give be enough information to let me decide if I need to define it or not. I read through all the ports that do define it, and none of them shed any light on to allow me to decide if my port needs to or not. So, first question is, are the 16 other ports that have interrupt functions and don’t define this just buggy (or non-optimal)? How can I tell if I need to or not? A single example of that does cause a port to see it and a single example of why a port would not need to define it would be instructive. Along the lines of TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS, I’d be willing to explain the required abi of each function, and then I just want gcc to just work. All functions are normal, except for interrupt functions, all allocatable register need to be saved. I think this is a universally true thing, and it just happens to be true on my port as well. Anyway, I’d love to see gcc improved in this area. This one is a pet peeve of mine as I’ve seen what happens to software when a single register that should have been saved, wasn’t. It was slightly annoying to track down and find. Fixing was trivial.
diff --git a/gcc/config/mips/mips-protos.h b/gcc/config/mips/mips-protos.h index 6ce3d70..e455553 100644 --- a/gcc/config/mips/mips-protos.h +++ b/gcc/config/mips/mips-protos.h @@ -317,6 +317,7 @@ extern unsigned int mips_sync_loop_insns (rtx, rtx *); extern const char *mips_output_division (const char *, rtx *); extern const char *mips_msa_output_division (const char *, rtx *); extern const char *mips_output_probe_stack_range (rtx, rtx); +extern bool mips_hard_regno_rename_ok (unsigned int, unsigned int); extern unsigned int mips_hard_regno_nregs (int, machine_mode); extern bool mips_linked_madd_p (rtx, rtx); extern bool mips_store_data_bypass_p (rtx, rtx); diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index a9829bd..151f774 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -12959,6 +12959,20 @@ mips_hard_regno_mode_ok_p (unsigned int regno, machine_mode mode) return false; } +/* Return nonzero if register OLD_REG can be renamed to register NEW_REG. */ + +bool +mips_hard_regno_rename_ok (unsigned int old_reg ATTRIBUTE_UNUSED, + unsigned int new_reg) +{ + /* Interrupt functions can only use registers that have already been + saved by the prologue, even if they would normally be call-clobbered. */ + if (cfun->machine->interrupt_handler_p && !df_regs_ever_live_p (new_reg)) + return false; + + return true; +} + /* Implement HARD_REGNO_NREGS. */ unsigned int diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index 6578ae5..3fe690a 100644 --- a/gcc/config/mips/mips.h +++ b/gcc/config/mips/mips.h @@ -1932,6 +1932,9 @@ struct mips_cpu_info { #define HARD_REGNO_MODE_OK(REGNO, MODE) \ mips_hard_regno_mode_ok[ (int)(MODE) ][ (REGNO) ] +#define HARD_REGNO_RENAME_OK(OLD_REG, NEW_REG) \ + mips_hard_regno_rename_ok (OLD_REG, NEW_REG) + /* Select a register mode required for caller save of hard regno REGNO. */ #define HARD_REGNO_CALLER_SAVE_MODE(REGNO, NREGS, MODE) \ mips_hard_regno_caller_save_mode (REGNO, NREGS, MODE) diff --git a/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c b/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c new file mode 100644 index 0000000..877d00c --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c @@ -0,0 +1,12 @@ +/* { dg-options "-funroll-loops" } */ +int foo; +int bar; + +void __attribute__ ((interrupt)) isr(void) +{ + if (!foo) + { + while (bar & 0xFF30); + } +} +/* { dg-final { scan-assembler-not "\\\$8" } } */