rs6000: Use SAVE_MULTIPLE only if we restore what it saves (PR80938)

Submitted by Segher Boessenkool on Aug. 9, 2017, 9:06 p.m.

Details

Message ID 21f6fe5be45ca917a46e204c4382c67ebfbb742f.1502310090.git.segher@kernel.crashing.org
State New
Headers show

Commit Message

Segher Boessenkool Aug. 9, 2017, 9:06 p.m.
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(+)

Comments

Alan Modra Aug. 10, 2017, 1:03 a.m.
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.
Segher Boessenkool Aug. 10, 2017, 2:28 a.m.
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
Alan Modra Aug. 10, 2017, 4:47 a.m.
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
Segher Boessenkool Aug. 10, 2017, 1:39 p.m.
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

Patch hide | download patch | download mbox

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.