| Message ID | 21f6fe5be45ca917a46e204c4382c67ebfbb742f.1502310090.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 09, 2017 at 09:06:18PM +0000, Segher Boessenkool wrote: > We can have SAVE_MULTIPLE while we do not have REST_MULTIPLE. If the > inline restore does not restore all registers, the CFI for the save > and restore can conflict if things are shrink-wrapped. > > We could restore all registers that are saved (not ideal), No, we can't do that. A -ffixed-reg register should not be restored even if it is saved, as such regs should never be written. For example, a fixed reg might be counting interrupts. If you restore it, you may lose count. Another example is a fixed reg used as some sort of context pointer. If you restore in a function that sets a new context you're obviously breaking user code. > or emit > the CFI notes to say we did (which will work fine, No, you can't do that either. Unwinding might then restore a -ffixed-reg register. > but is also not > so great); instead, let's not save the registers that are unused. Ick, looks like papering over the real problem to me, and will no doubt cause -Os size regressions. Also, if SAVE_MULTIPLE causes this bad interaction with shrink-wrap, does using the out-of-line register save functions cause the same problem? I haven't looked yet, but at a guess I suspect the correct solution is to modify cfa_restores in rs6000_emit_epilogue.
On Thu, Aug 10, 2017 at 10:33:05AM +0930, Alan Modra wrote: > On Wed, Aug 09, 2017 at 09:06:18PM +0000, Segher Boessenkool wrote: > > We can have SAVE_MULTIPLE while we do not have REST_MULTIPLE. If the > > inline restore does not restore all registers, the CFI for the save > > and restore can conflict if things are shrink-wrapped. > > > > We could restore all registers that are saved (not ideal), > > No, we can't do that. A -ffixed-reg register should not be restored > even if it is saved, as such regs should never be written. For > example, a fixed reg might be counting interrupts. If you restore it, > you may lose count. Another example is a fixed reg used as some sort > of context pointer. If you restore in a function that sets a new > context you're obviously breaking user code. Yes, sorry for glossing over this. We need to handle fixed registers specially in most other prologue/epilogue things, too (and we hopefully do everywhere it is needed). > > or emit > > the CFI notes to say we did (which will work fine, > > No, you can't do that either. Unwinding might then restore a > -ffixed-reg register. Yep. > > but is also not > > so great); instead, let's not save the registers that are unused. > > Ick, looks like papering over the real problem to me, and will no > doubt cause -Os size regressions. I think it is very directly solving the real problem. It isn't likely to cause size regressions (look how long it took for this PR to show up, so this cannot happen often). This only happens if r30 (the PIC reg) is used but r31 isn't; which means that a) there are no other registers to save, or b) the function is marked as needing a hard frame pointer but eventually doesn't need one. (RA picks the registers r31, r30, ... in sequence). In the case in the PR, this patch replaces one insn by one (cheaper) insn. > Also, if SAVE_MULTIPLE causes this > bad interaction with shrink-wrap, does using the out-of-line register > save functions cause the same problem? I do not know. Not with the code in the PR (restoring only one or two registers isn't done out-of-line, and it's a sibcall exit as well). Segher
On Wed, Aug 09, 2017 at 09:28:22PM -0500, Segher Boessenkool wrote: > On Thu, Aug 10, 2017 at 10:33:05AM +0930, Alan Modra wrote: > > On Wed, Aug 09, 2017 at 09:06:18PM +0000, Segher Boessenkool wrote: > > > We can have SAVE_MULTIPLE while we do not have REST_MULTIPLE. If the > > > inline restore does not restore all registers, the CFI for the save > > > and restore can conflict if things are shrink-wrapped. > > > > > > We could restore all registers that are saved (not ideal), > > > > No, we can't do that. A -ffixed-reg register should not be restored > > even if it is saved, as such regs should never be written. For > > example, a fixed reg might be counting interrupts. If you restore it, > > you may lose count. Another example is a fixed reg used as some sort > > of context pointer. If you restore in a function that sets a new > > context you're obviously breaking user code. > > Yes, sorry for glossing over this. We need to handle fixed registers > specially in most other prologue/epilogue things, too (and we hopefully > do everywhere it is needed). We don't. :-( I have a fix and will post after testing. > > > or emit > > > the CFI notes to say we did (which will work fine, > > > > No, you can't do that either. Unwinding might then restore a > > -ffixed-reg register. > > Yep. > > > > but is also not > > > so great); instead, let's not save the registers that are unused. > > > > Ick, looks like papering over the real problem to me, and will no > > doubt cause -Os size regressions. > > I think it is very directly solving the real problem. It isn't likely > to cause size regressions (look how long it took for this PR to show > up, so this cannot happen often). > > This only happens if r30 (the PIC reg) is used but r31 isn't; which means > that a) there are no other registers to save, or b) the function is marked > as needing a hard frame pointer but eventually doesn't need one. > > (RA picks the registers r31, r30, ... in sequence). > > In the case in the PR, this patch replaces one insn by one (cheaper) > insn. And in other cases your patch will prevent stmw when it should be used. Testcase attached. It shows the wrong use of lmw too. > > Also, if SAVE_MULTIPLE causes this > > bad interaction with shrink-wrap, does using the out-of-line register > > save functions cause the same problem? > > I do not know. Not with the code in the PR (restoring only one or two > registers isn't done out-of-line, and it's a sibcall exit as well). > > > Segher
On Thu, Aug 10, 2017 at 02:17:40PM +0930, Alan Modra wrote: > > > Ick, looks like papering over the real problem to me, and will no > > > doubt cause -Os size regressions. > > > > I think it is very directly solving the real problem. It isn't likely > > to cause size regressions (look how long it took for this PR to show > > up, so this cannot happen often). > > > > This only happens if r30 (the PIC reg) is used but r31 isn't; which means > > that a) there are no other registers to save, or b) the function is marked > > as needing a hard frame pointer but eventually doesn't need one. > > > > (RA picks the registers r31, r30, ... in sequence). > > > > In the case in the PR, this patch replaces one insn by one (cheaper) > > insn. > > And in other cases your patch will prevent stmw when it should be > used. Testcase attached. It shows the wrong use of lmw too. I dunno... If you change r25 to r14 in that testcase it will use stmw 14 with my patch reverted. Not very reasonable imho (but then again, people using register asm like this get what they deserve anyway). Segher
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 0f0b1ff..e8cdd25 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -24437,6 +24437,21 @@ rs6000_savres_strategy (rs6000_stack_t *info, else if (!lr_save_p && info->first_gp_reg_save > 29) strategy |= SAVE_INLINE_GPRS | REST_INLINE_GPRS; + /* We can only use save multiple if we need to save all the registers from + first_gp_reg_save. Otherwise, the CFI gets messed up (we save some + register we do not restore). */ + if (strategy & SAVE_MULTIPLE) + { + int i; + + for (i = info->first_gp_reg_save; i < 32; i++) + if (fixed_reg_p (i) || !save_reg_p (i)) + { + strategy &= ~SAVE_MULTIPLE; + break; + } + } + /* We can only use load multiple or the out-of-line routines to restore gprs if we've saved all the registers from first_gp_reg_save. Otherwise, we risk loading garbage.
We can have SAVE_MULTIPLE while we do not have REST_MULTIPLE. If the inline restore does not restore all registers, the CFI for the save and restore can conflict if things are shrink-wrapped. We could restore all registers that are saved (not ideal), or emit the CFI notes to say we did (which will work fine, but is also not so great); instead, let's not save the registers that are unused. Tested on powerpc64-linux {-m32,-m64}; committing to trunk. Segher 2017-08-09 Segher Boessenkool <segher@kernel.crashing.org> PR target/80938 * config/rs6000/rs6000.c (rs6000_savres_strategy): Don't use SAVE_MULTIPLE if not all the registers that saves, should be saved. --- gcc/config/rs6000/rs6000.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)