Re: [PATCH] rs6000: Use SAVE_MULTIPLE only if we restore what it saves (PR80938)
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
Re: [PATCH] rs6000: Use SAVE_MULTIPLE only if we restore what it saves (PR80938)
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 +, 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
--
Alan Modra
Australia Development Lab, IBM
#ifdef USER_R26
register long r26 __asm__ ("r26");
#endif
int f1 (void)
{
register long r25 __asm__ ("r25");
register long r27 __asm__ ("r27");
register long r28 __asm__ ("r28");
register long r29 __asm__ ("r29");
register long r31 __asm__ ("r31");
__asm__ __volatile__
("#uses %0 %1 %2 %3 %4"
: "=r" (r25), "=r" (r27), "=r" (r28), "=r" (r29), "=r" (r31));
return 0;
}
void f2 (char *s)
{
int col = 0;
char c;
void f3 (char c)
{
extern int f4 (char);
register long r25 __asm__ ("r25");
register long r27 __asm__ ("r27");
register long r28 __asm__ ("r28");
register long r29 __asm__ ("r29");
register long r31 __asm__ ("r31");
if (c == '\t')
do f3 (' '); while (col % 8 != 0);
else
{
__asm__ __volatile__
("#uses %0 %1 %2 %3 %4"
: "=r" (r25), "=r" (r27), "=r" (r28), "=r" (r29), "=r" (r31));
f4 (c);
col++;
}
}
while ((c = *s++) != 0)
f3 (c);
}
Re: [PATCH] rs6000: Use SAVE_MULTIPLE only if we restore what it saves (PR80938)
On Thu, Aug 10, 2017 at 10:33:05AM +0930, Alan Modra wrote: > On Wed, Aug 09, 2017 at 09:06:18PM +, 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
Re: [PATCH] rs6000: Use SAVE_MULTIPLE only if we restore what it saves (PR80938)
On Wed, Aug 09, 2017 at 09:06:18PM +, 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. -- Alan Modra Australia Development Lab, IBM
