[RS6000] Fix PR35907, vmx regs trashed

[RS6000] Fix PR35907, vmx regs trashed

[Alan Modra]

This patch cures an oversight in Eric's 2007-05-27 change that could
cause vmx registers to be restored from the wrong location if a
function called alloca, and reinstates a fixed version of Eric's code
to restore vrsave that I reverted 2007-10-20 for PR33812.  The
reversion ignored the fact that vrsave can live outside the area of
stack valid below sp.

So, we now have vmx registers being restored either before or after
the frame pop, depending on their location in the frame.  We could
always restore before the frame pop, but this can cost one extra
reg->reg move instruction when use_backchain_to_restore_sp.  (It's
possible to restore vmx regs after the frame pop even when
use_backchain_to_restore_sp, if the vmx regs are within the area of
stack valid below sp.  If we restore before the pop in this case then
we'll load the backchain into r11 then move to r1, whereas restoring
after the pop will load the backchain directly into r1.)

Bootstrapped and regression tested powerpc-linux and powerpc64-linux.
OK to apply both mainline and 4.3?

	PR target/35907
	* config/rs6000/rs6000.c (rs6000_emit_epilogue): Restore vr and vrsave
	regs before frame pop when needed.  If use_backchain_to_restore_sp
	then load backchain into a temp reg to restore vr and vrsave.  Add
	code to restore vr after frame pop if possible.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 134338)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -16370,11 +16370,23 @@ rs6000_emit_epilogue (int sibcall)
   if (info->push_p)
     sp_offset = info->total_size;
 
-  /* Restore AltiVec registers if needed.  */
-  if (TARGET_ALTIVEC_ABI && info->altivec_size != 0)
+  /* Restore AltiVec registers if we must do so before adjusting the
+     stack.  */
+  if (TARGET_ALTIVEC_ABI
+      && info->altivec_size != 0
+      && DEFAULT_ABI != ABI_V4
+      && info->altivec_save_offset < (TARGET_32BIT ? -220 : -288))
     {
       int i;
 
+      if (use_backchain_to_restore_sp)
+	{
+	  frame_reg_rtx = gen_rtx_REG (Pmode, 11);
+	  emit_move_insn (frame_reg_rtx,
+			  gen_rtx_MEM (Pmode, sp_reg_rtx));
+	  sp_offset = 0;
+	}
+
       for (i = info->first_altivec_reg_save; i <= LAST_ALTIVEC_REGNO; ++i)
 	if (info->vrsave_mask & ALTIVEC_REG_BIT (i))
 	  {
@@ -16394,19 +16406,54 @@ rs6000_emit_epilogue (int sibcall)
 	  }
     }
 
+  /* Restore VRSAVE if we must do so before adjusting the stack.  */
+  if (TARGET_ALTIVEC
+      && TARGET_ALTIVEC_VRSAVE
+      && info->vrsave_mask != 0
+      && DEFAULT_ABI != ABI_V4
+      && info->vrsave_save_offset < (TARGET_32BIT ? -220 : -288))
+    {
+      rtx addr, mem, reg;
+
+      if (use_backchain_to_restore_sp
+	  && frame_reg_rtx == sp_reg_rtx)
+	{
+	  frame_reg_rtx = gen_rtx_REG (Pmode, 11);
+	  emit_move_insn (frame_reg_rtx,
+			  gen_rtx_MEM (Pmode, sp_reg_rtx));
+	  sp_offset = 0;
+	}
+
+      addr = gen_rtx_PLUS (Pmode, frame_reg_rtx,
+			   GEN_INT (info->vrsave_save_offset + sp_offset));
+      mem = gen_frame_mem (SImode, addr);
+      reg = gen_rtx_REG (SImode, 12);
+      emit_move_insn (reg, mem);
+
+      emit_insn (generate_set_vrsave (reg, info, 1));
+    }
+
   /* If we have a frame pointer, a call to alloca,  or a large stack
      frame, restore the old stack pointer using the backchain.  Otherwise,
      we know what size to update it with.  */
   if (use_backchain_to_restore_sp)
     {
-      /* Under V.4, don't reset the stack pointer until after we're done
-	 loading the saved registers.  */
-      if (DEFAULT_ABI == ABI_V4)
-	frame_reg_rtx = gen_rtx_REG (Pmode, 11);
+      if (frame_reg_rtx != sp_reg_rtx)
+	{
+	  emit_move_insn (sp_reg_rtx, frame_reg_rtx);
+	  frame_reg_rtx = sp_reg_rtx;
+	}
+      else
+	{
+	  /* Under V.4, don't reset the stack pointer until after we're done
+	     loading the saved registers.  */
+	  if (DEFAULT_ABI == ABI_V4)
+	    frame_reg_rtx = gen_rtx_REG (Pmode, 11);
 
-      emit_move_insn (frame_reg_rtx,
-		      gen_rtx_MEM (Pmode, sp_reg_rtx));
-      sp_offset = 0;
+	  emit_move_insn (frame_reg_rtx,
+			  gen_rtx_MEM (Pmode, sp_reg_rtx));
+	  sp_offset = 0;
+	}
     }
   else if (info->push_p
 	   && DEFAULT_ABI != ABI_V4
@@ -16420,9 +16467,39 @@ rs6000_emit_epilogue (int sibcall)
       sp_offset = 0;
     }
 
-  /* Restore VRSAVE if needed.  */
-  if (TARGET_ALTIVEC && TARGET_ALTIVEC_VRSAVE
-      && info->vrsave_mask != 0)
+  /* Restore AltiVec registers if we have not done so already.  */
+  if (TARGET_ALTIVEC_ABI
+      && info->altivec_size != 0
+      && (DEFAULT_ABI == ABI_V4
+	  || info->altivec_save_offset >= (TARGET_32BIT ? -220 : -288)))
+    {
+      int i;
+
+      for (i = info->first_altivec_reg_save; i <= LAST_ALTIVEC_REGNO; ++i)
+	if (info->vrsave_mask & ALTIVEC_REG_BIT (i))
+	  {
+	    rtx addr, areg, mem;
+
+	    areg = gen_rtx_REG (Pmode, 0);
+	    emit_move_insn
+	      (areg, GEN_INT (info->altivec_save_offset
+			      + sp_offset
+			      + 16 * (i - info->first_altivec_reg_save)));
+
+	    /* AltiVec addressing mode is [reg+reg].  */
+	    addr = gen_rtx_PLUS (Pmode, frame_reg_rtx, areg);
+	    mem = gen_frame_mem (V4SImode, addr);
+
+	    emit_move_insn (gen_rtx_REG (V4SImode, i), mem);
+	  }
+    }
+
+  /* Restore VRSAVE if we have not done so already.  */
+  if (TARGET_ALTIVEC
+      && TARGET_ALTIVEC_VRSAVE
+      && info->vrsave_mask != 0
+      && (DEFAULT_ABI == ABI_V4
+	  || info->vrsave_save_offset >= (TARGET_32BIT ? -220 : -288)))
     {
       rtx addr, mem, reg;
 

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RS6000] Fix PR35907, vmx regs trashed

[David Edelsohn]

Alan,

	The patch is fine.  Thanks for detangling this!

	We separately can have the discussion about whether always
restoring before the frame pop is worth changing.

Thanks, David

[testcase] Fix PR35907, vmx regs trashed

[Jakub Jelinek]

Hi!

On Wed, Apr 16, 2008 at 08:21:04PM -0400, David Edelsohn wrote:
> 	The patch is fine.  Thanks for detangling this!

The patch was committed without a testcase, is this ok for trunk/4.3?
Regtested on both:

2008-04-17  Jakub Jelinek  <jakub@redhat.com>
	    Peter Bergner  <bergner@vnet.ibm.com>

	PR target/35907
	* gcc.target/powerpc/pr35907.c: New test.

--- gcc/testsuite/gcc.target/powerpc/pr35907.c.jj	2008-04-16 10:04:23.000000000 +0200
+++ gcc/testsuite/gcc.target/powerpc/pr35907.c	2008-04-16 10:04:58.000000000 +0200
@@ -0,0 +1,59 @@
+/* PR target/35907 */
+/* { dg-do run { target powerpc*-*-* } } */
+/* { dg-require-effective-target powerpc_altivec_ok } */
+/* { dg-options "-O2 -maltivec" } */
+
+#include "altivec_check.h"
+
+#define vector __attribute__((vector_size (16)))
+union
+{
+  vector int k;
+  int c[16];
+} u, v, w;
+vector int m;
+
+void __attribute__((noinline))
+bar (void *i, vector int j)
+{
+  asm volatile ("" : : "r" (i), "r" (&j) : "memory");
+}
+
+int __attribute__((noinline))
+foo (int i, vector int j)
+{
+  char *p = __builtin_alloca (64 + i);
+  m += u.k;
+  v.k = m;
+  w.k = j;
+  if (__builtin_memcmp (&v.c, &w.c, 16) != 0)
+    __builtin_abort ();
+  j += u.k;
+  bar (p, j);
+  j += u.k;
+  bar (p, j);
+  return 0;
+}
+
+void
+test (void)
+{
+  vector int l;
+  int i;
+  for (i = 0; i < 4; i++)
+    u.c[i] = i;
+  l = u.k;
+  if (foo (64, l))
+    __builtin_abort ();
+  l += u.k;
+  if (foo (64, l))
+    __builtin_abort ();
+}
+
+int
+main ()
+{
+  altivec_check ();
+  test ();
+  return 0;
+}


	Jakub

Re: [RS6000] Fix PR35907, vmx regs trashed

[Alan Modra]

This implements register restore and frame pop via the frame pointer
if we happen to have one, which is slightly better than using the
backchain.  I've also introduced ALWAYS_RESTORE_ALTIVEC_BEFORE_POP
and the changes necessary to do as the macro says.  The idea here is
to make it easy to test whether it is worth having two copies of the
vmx register restore code in rs6000_emit_epilogue.  I guess it isn't
that much of a maintenance burden, but we only gain one instruction
and then only when we have a large frame but don't use enough
registers to put the vmx area outside of valid stack below sp.

Bootstrapped and regression tested powerpc-linux and powerpc64-linux,
both ALWAYS_RESTORE_ALTIVEC_BEFORE_POP and
!ALWAYS_RESTORE_ALTIVEC_BEFORE_POP.  OK for mainline?

	* config/rs6000/rs6000.c (ALWAYS_RESTORE_ALTIVEC_BEFORE_POP): Define.
	(rs6000_emit_epilogue): Use backchain to restore only when we
	have a large frame.  Make use of frame pointer to restore if we
	have one.  Handle ALWAYS_RESTORE_ALTIVEC_BEFORE_POP.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 134387)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -16228,6 +16228,10 @@ rs6000_output_function_prologue (FILE *f
   rs6000_pic_labelno++;
 }
 
+/* Non-zero if vmx regs are restored before the frame pop, zero if
+   we restore after the pop when possible.  */
+#define ALWAYS_RESTORE_ALTIVEC_BEFORE_POP 0
+
 /* Emit function epilogue as insns.
 
    At present, dwarf2out_frame_debug_expr doesn't understand
@@ -16267,13 +16271,18 @@ rs6000_emit_epilogue (int sibcall)
 			   || current_function_calls_eh_return
 			   || info->first_fp_reg_save == 64
 			   || FP_SAVE_INLINE (info->first_fp_reg_save));
-  use_backchain_to_restore_sp = (frame_pointer_needed
-				 || current_function_calls_alloca
-				 || info->total_size > 32767);
   using_mtcr_multiple = (rs6000_cpu == PROCESSOR_PPC601
 			 || rs6000_cpu == PROCESSOR_PPC603
 			 || rs6000_cpu == PROCESSOR_PPC750
 			 || optimize_size);
+  /* Restore via the backchain when we have a large frame, since this
+     is more efficient than an addis, addi pair.  The second condition
+     here will not trigger at the moment;  We don't actually need a
+     frame pointer for alloca, but the generic parts of the compiler
+     give us one anyway.  */
+  use_backchain_to_restore_sp = (info->total_size > 32767
+				 || (current_function_calls_alloca
+				     && !frame_pointer_needed));
 
   if (WORLD_SAVE_P (info))
     {
@@ -16374,8 +16383,9 @@ rs6000_emit_epilogue (int sibcall)
      stack.  */
   if (TARGET_ALTIVEC_ABI
       && info->altivec_size != 0
-      && DEFAULT_ABI != ABI_V4
-      && info->altivec_save_offset < (TARGET_32BIT ? -220 : -288))
+      && (ALWAYS_RESTORE_ALTIVEC_BEFORE_POP
+	  || (DEFAULT_ABI != ABI_V4
+	      && info->altivec_save_offset < (TARGET_32BIT ? -220 : -288))))
     {
       int i;
 
@@ -16386,6 +16396,8 @@ rs6000_emit_epilogue (int sibcall)
 			  gen_rtx_MEM (Pmode, sp_reg_rtx));
 	  sp_offset = 0;
 	}
+      else if (frame_pointer_needed)
+	frame_reg_rtx = hard_frame_pointer_rtx;
 
       for (i = info->first_altivec_reg_save; i <= LAST_ALTIVEC_REGNO; ++i)
 	if (info->vrsave_mask & ALTIVEC_REG_BIT (i))
@@ -16410,18 +16422,23 @@ rs6000_emit_epilogue (int sibcall)
   if (TARGET_ALTIVEC
       && TARGET_ALTIVEC_VRSAVE
       && info->vrsave_mask != 0
-      && DEFAULT_ABI != ABI_V4
-      && info->vrsave_save_offset < (TARGET_32BIT ? -220 : -288))
+      && (ALWAYS_RESTORE_ALTIVEC_BEFORE_POP
+	  || (DEFAULT_ABI != ABI_V4
+	      && info->vrsave_save_offset < (TARGET_32BIT ? -220 : -288))))
     {
       rtx addr, mem, reg;
 
-      if (use_backchain_to_restore_sp
-	  && frame_reg_rtx == sp_reg_rtx)
+      if (frame_reg_rtx == sp_reg_rtx)
 	{
-	  frame_reg_rtx = gen_rtx_REG (Pmode, 11);
-	  emit_move_insn (frame_reg_rtx,
-			  gen_rtx_MEM (Pmode, sp_reg_rtx));
-	  sp_offset = 0;
+	  if (use_backchain_to_restore_sp)
+	    {
+	      frame_reg_rtx = gen_rtx_REG (Pmode, 11);
+	      emit_move_insn (frame_reg_rtx,
+			      gen_rtx_MEM (Pmode, sp_reg_rtx));
+	      sp_offset = 0;
+	    }
+	  else if (frame_pointer_needed)
+	    frame_reg_rtx = hard_frame_pointer_rtx;
 	}
 
       addr = gen_rtx_PLUS (Pmode, frame_reg_rtx,
@@ -16433,17 +16450,11 @@ rs6000_emit_epilogue (int sibcall)
       emit_insn (generate_set_vrsave (reg, info, 1));
     }
 
-  /* If we have a frame pointer, a call to alloca,  or a large stack
-     frame, restore the old stack pointer using the backchain.  Otherwise,
-     we know what size to update it with.  */
+  /* If we have a large stack frame, restore the old stack pointer
+     using the backchain.  */
   if (use_backchain_to_restore_sp)
     {
-      if (frame_reg_rtx != sp_reg_rtx)
-	{
-	  emit_move_insn (sp_reg_rtx, frame_reg_rtx);
-	  frame_reg_rtx = sp_reg_rtx;
-	}
-      else
+      if (frame_reg_rtx == sp_reg_rtx)
 	{
 	  /* Under V.4, don't reset the stack pointer until after we're done
 	     loading the saved registers.  */
@@ -16454,6 +16465,30 @@ rs6000_emit_epilogue (int sibcall)
 			  gen_rtx_MEM (Pmode, sp_reg_rtx));
 	  sp_offset = 0;
 	}
+      else if (ALWAYS_RESTORE_ALTIVEC_BEFORE_POP
+	       && DEFAULT_ABI == ABI_V4)
+	/* frame_reg_rtx has been set up by the altivec restore.  */
+	;
+      else
+	{
+	  emit_move_insn (sp_reg_rtx, frame_reg_rtx);
+	  frame_reg_rtx = sp_reg_rtx;
+	}
+    }
+  /* If we have a frame pointer, we can restore the old stack pointer
+     from it.  */
+  else if (frame_pointer_needed)
+    {
+      frame_reg_rtx = sp_reg_rtx;
+      if (DEFAULT_ABI == ABI_V4)
+	frame_reg_rtx = gen_rtx_REG (Pmode, 11);
+
+      emit_insn (TARGET_32BIT
+		 ? gen_addsi3 (frame_reg_rtx, hard_frame_pointer_rtx,
+			       GEN_INT (info->total_size))
+		 : gen_adddi3 (frame_reg_rtx, hard_frame_pointer_rtx,
+			       GEN_INT (info->total_size)));
+      sp_offset = 0;
     }
   else if (info->push_p
 	   && DEFAULT_ABI != ABI_V4
@@ -16468,7 +16503,8 @@ rs6000_emit_epilogue (int sibcall)
     }
 
   /* Restore AltiVec registers if we have not done so already.  */
-  if (TARGET_ALTIVEC_ABI
+  if (!ALWAYS_RESTORE_ALTIVEC_BEFORE_POP
+      && TARGET_ALTIVEC_ABI
       && info->altivec_size != 0
       && (DEFAULT_ABI == ABI_V4
 	  || info->altivec_save_offset >= (TARGET_32BIT ? -220 : -288)))
@@ -16495,7 +16531,8 @@ rs6000_emit_epilogue (int sibcall)
     }
 
   /* Restore VRSAVE if we have not done so already.  */
-  if (TARGET_ALTIVEC
+  if (!ALWAYS_RESTORE_ALTIVEC_BEFORE_POP
+      && TARGET_ALTIVEC
       && TARGET_ALTIVEC_VRSAVE
       && info->vrsave_mask != 0
       && (DEFAULT_ABI == ABI_V4

-- 
Alan Modra
Australia Development Lab, IBM

Re: [testcase] Fix PR35907, vmx regs trashed

[David Edelsohn]

2008-04-17  Jakub Jelinek  <jakub@redhat.com>
	    Peter Bergner  <bergner@vnet.ibm.com>

	PR target/35907
	* gcc.target/powerpc/pr35907.c: New test.


Thanks for the testcase.  It's fine everywhere.

Thanks, David

Re: [RS6000] Fix PR35907, vmx regs trashed

[David Edelsohn]

	* config/rs6000/rs6000.c (ALWAYS_RESTORE_ALTIVEC_BEFORE_POP): Define.
	(rs6000_emit_epilogue): Use backchain to restore only when we
	have a large frame.  Make use of frame pointer to restore if we
	have one.  Handle ALWAYS_RESTORE_ALTIVEC_BEFORE_POP.

Okay.

David