| Message ID | B5E67142681B53468FAF6B7C31356562441B05D2@hhmail02.hh.imgtec.org |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: Robert Suchanek [mailto:Robert.Suchanek@imgtec.com] > Sent: Friday, August 14, 2015 8:01 AM > To: Mike Stump; Richard Sandiford > Cc: Moore, Catherine; Matthew Fortune; gcc-patches@gcc.gnu.org > Subject: RE: [PATCH][MIPS] Fix register renaming in the interrupt handlers > > Hi, > > > > 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. > > Indeed. I naively thought that there would be a single place that needed an > update and overlooked this hook. > Sorry for missing this as well. > > gcc/ > * config/mips/mips-protos.h (mips_hard_regno_rename_ok): New > prototype. > * config/mips/mips.c (mips_hard_regno_rename_ok): New > function. > (mips_hard_regno_scratch_ok): Likewise. > (TARGET_HARD_REGNO_SCRATCH_OK): Define macro. > * config/mips/mips.h (HARD_REGNO_RENAME_OK): New. > > gcc/testsuite/ > * gcc.target/mips/interrupt_handler-bug-1.c: New test. > --- Based on the feedback from Richard and Mike, this looks OK now. Thanks, Catherine
Robert Suchanek <Robert.Suchanek@imgtec.com> writes: >> > 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. > > Indeed. I naively thought that there would be a single place that needed > an update and overlooked this hook. > >> 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. The port is only buggy if they have define_peephole2s with match_scratches and those match_scratches require a register that would be saved by an interrupt function. In other cases defining T_H_R_S_O would have no effect (but still be a good idea for future proofing -- obviously someone who adds a new define_peephole2 is unlikely to be thinking about inerrupt functions). >> 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. > > The cleanup as Richard suggested would probably be a good start. It's > interesting that this kind of bug wasn't discovered earlier and likely > existing for several years. I think the underlying problem is that target-independent code knows which set of registers are call(ee)-saved under the ABI, but doesn't know about function-specific variations. The idea was to expose that directly as a new register set in the main function struct. We can then get rid of all definitions of TARGET_HARD_REGNO_SCRATCH_OK and most definitions of HARD_REGNO_RENAME_OK. (Some renames are invalid for other reasons.) The main problem is that we then have three sets of registers: - the ABI call-clobbered set - the call-clobbered set for the current function - the set of registers that are clobbered by an already-compiled function (for -fipa-ra) I think that's valid, but you obviously have to be very careful to use the right one. Especially when it comes to cached derived information. Thanks, Richard
On Aug 15, 2015, at 3:32 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > > The port is only buggy if they have define_peephole2s with match_scratches > and those match_scratches require a register that would be saved by > an interrupt function. In other cases defining T_H_R_S_O would have > no effect (but still be a good idea for future proofing -- obviously > someone who adds a new define_peephole2 is unlikely to be thinking > about inerrupt functions). Yeah, that was my reading of the code after I posted as well. My port was buggy. :-( I think all the other ports like likely buggy or suboptimal. > The main problem is that we then have three sets of registers: > - the ABI call-clobbered set > - the call-clobbered set for the current function > - the set of registers that are clobbered by an already-compiled function > (for -fipa-ra) > > I think that's valid, but you obviously have to be very careful to use > the right one. Especially when it comes to cached derived information. No. Being very careful isn’t the right solution. It should be impossible to use the wrong one. Something like static inline call_used (regno, call_abi = 0) { return call_used[call_abi][regno]; } where callers that want the call_used for a given function would call call_used (regno, fndecl) and fndecl can convert over to a call_abi or possibly something like fndecl->call_used[regno] where most callers can just use cfun->call_used[regnp] is the right one. It is fine for ports without interrupt functions (because call_abi defaults to 0), and it is the right one to use on ports with interrupt functions. In the later, the interrupt function will have its call_abi set appropriately. Ports that already have and manage call_abi today will notice they even like the new scheme. And the TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS code I think could benefit from this as well.
Mike Stump <mikestump@comcast.net> writes: > On Aug 15, 2015, at 3:32 AM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> >> The port is only buggy if they have define_peephole2s with match_scratches >> and those match_scratches require a register that would be saved by >> an interrupt function. In other cases defining T_H_R_S_O would have >> no effect (but still be a good idea for future proofing -- obviously >> someone who adds a new define_peephole2 is unlikely to be thinking >> about inerrupt functions). > > Yeah, that was my reading of the code after I posted as well. My port > was buggy. :-( I think all the other ports like likely buggy or > suboptimal. Suboptimal how? >> The main problem is that we then have three sets of registers: >> - the ABI call-clobbered set >> - the call-clobbered set for the current function >> - the set of registers that are clobbered by an already-compiled function >> (for -fipa-ra) >> >> I think that's valid, but you obviously have to be very careful to use >> the right one. Especially when it comes to cached derived information. > > No. Being very careful isn’t the right solution. It should be > impossible to use the wrong one. > > Something like static inline call_used (regno, call_abi = 0) { return > call_used[call_abi][regno]; } where callers that want the call_used for > a given function would call call_used (regno, fndecl) and fndecl can > convert over to a call_abi or possibly something like > fndecl->call_used[regno] where most callers can just use > cfun->call_used[regnp] is the right one. It is fine for ports without > interrupt functions (because call_abi defaults to 0), and it is the > right one to use on ports with interrupt functions. In the later, the > interrupt function will have its call_abi set appropriately. Ports that > already have and manage call_abi today will notice they even like the > new scheme. And the TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS > code I think could benefit from this as well. I don't see how that helps. The default argument creates exactly the same difference as thw first two in my list. There are many times when you don't know which function is being called and so can't pass a decl. The third in my list is still there unless you disable -fipa-ra. Richard
On Aug 15, 2015, at 9:19 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Mike Stump <mikestump@comcast.net> writes: >> On Aug 15, 2015, at 3:32 AM, Richard Sandiford >> <rdsandiford@googlemail.com> wrote: >>> >>> The port is only buggy if they have define_peephole2s with match_scratches >>> and those match_scratches require a register that would be saved by >>> an interrupt function. In other cases defining T_H_R_S_O would have >>> no effect (but still be a good idea for future proofing -- obviously >>> someone who adds a new define_peephole2 is unlikely to be thinking >>> about inerrupt functions). >> >> Yeah, that was my reading of the code after I posted as well. My port >> was buggy. :-( I think all the other ports like likely buggy or >> suboptimal. > > Suboptimal how? Inability to use some registers. Hum, maybe that can’t happen for other reasons. > I don't see how that helps. Maybe I’m envisioning something you aren’t proposing. Anyway, a nice fix for this has code like: /* If this is a CALL_INSN, all call used registers are stored with unknown values. */ if (CALL_P (insn)) { for (i = FIRST_PSEUDO_REGISTER - 1; i >= 0; i--) { if (call_used_regs[i]) /* Reset the information about this register. */ reg_mode[i] = VOIDmode; } } being updated so that at least for some calls to an interrupt function, the mode of the reg isn’t set to VOIDmode. I didn’t choose this for any specific reason, I just grabbed a random use of call_used_regs that likely is wrong or less than optimal. In the type of fix I envision, it would/could address this. > The third in my list is still there unless you disable -fipa-ra. Maybe I was railing against something you’re not proposing. Anyway, safe to defer til we have more detail.
Hi, > > gcc/ > > * config/mips/mips-protos.h (mips_hard_regno_rename_ok): New > > prototype. > > * config/mips/mips.c (mips_hard_regno_rename_ok): New > > function. > > (mips_hard_regno_scratch_ok): Likewise. > > (TARGET_HARD_REGNO_SCRATCH_OK): Define macro. > > * config/mips/mips.h (HARD_REGNO_RENAME_OK): New. > > > > gcc/testsuite/ > > * gcc.target/mips/interrupt_handler-bug-1.c: New test. > > --- > > Based on the feedback from Richard and Mike, this looks OK now. > Thanks, > Catherine Committed as r226968. I slightly modified the test to search for "^isr:.*\\\$8.*isr" instead of just "\\\$8" as with the ToT compiler 2 tests failed with -flto. It happened that $8 appeared in the LTO bytecode. Changed as obvious. Regards, Robert
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..d0851e9 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -12959,6 +12959,33 @@ 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; +} + +/* Return nonzero if register REGNO can be used as a scratch register + in peephole2. */ + +bool +mips_hard_regno_scratch_ok (unsigned int regno) +{ + /* See mips_hard_regno_rename_ok. */ + if (cfun->machine->interrupt_handler_p && !df_regs_ever_live_p (regno)) + return false; + + return true; +} + /* Implement HARD_REGNO_NREGS. */ unsigned int @@ -22428,6 +22455,9 @@ mips_lra_p (void) #undef TARGET_SCHED_SET_SCHED_FLAGS #define TARGET_SCHED_SET_SCHED_FLAGS mips_set_sched_flags +#undef TARGET_HARD_REGNO_SCRATCH_OK +#define TARGET_HARD_REGNO_SCRATCH_OK mips_hard_regno_scratch_ok + struct gcc_target targetm = TARGET_INITIALIZER; #include "gt-mips.h" 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..128ba90 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c @@ -0,0 +1,11 @@ +/* { dg-options "-funroll-loops" } */ +int foo; +int bar; + +void __attribute__ ((interrupt)) +isr (void) +{ + if (!foo) + while (bar & 0xFF30); +} +/* { dg-final { scan-assembler-not "\\\$8" } } */