[PATCH,rs6000] fix interrupt safety issue on E500 targets

[PATCH,rs6000] fix interrupt safety issue on E500 targets

[Nathan Froyd]

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]

The attached patch fixes an interrupt safety problem on E500 targets;
this problem was introduced by the ABI fixes that were committed several
months ago.  On SPE targets we would generate the following code for a
function epilogue:

    point r11 into the stack frame
    restore registers
    restore stack pointer
    return

and the instruction scheduler, being clever, would turn this into:

    point r11 into the stack frame
    restore a few registers
    restore stack pointer
    restore remaining registers		(XXX)
    return

Which causes problems if an interrupt happens during XXX because the OS
will clobber the space from which we are restoring registers, believing
it to be unused.

The fix is simple.  We just have to tell the scheduler that we
have a blocking insn just before the restore of the stack pointer.

Tested on powerpc-none-linux-gnuspe with no regressions.  OK to commit?

-Nathan

2007-10-03  Nathan Froyd  <froydnj@codesourcery.com>

	gcc/
	* config/rs6000/rs6000.c (rs6000_emit_epilogue): Emit a stack tie
	when compiling for SPE targets due to r11 pointing into the frame.


[-- Attachment #2: r14-add-insn.patch --]
[-- Type: text/x-diff, Size: 999 bytes --]

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 128981)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -16329,13 +16329,18 @@ rs6000_emit_epilogue (int sibcall)
 	    }
     }
 
+  if (frame_reg_rtx != sp_reg_rtx
+      || (TARGET_SPE_ABI
+          && info->spe_64bit_regs_used != 0
+          && info->first_gp_reg_save != 32))
+    /* This blockage is needed so that sched doesn't decide to move
+       the sp change before the register restores.  */
+    rs6000_emit_stack_tie ();
+    
   /* If this is V.4, unwind the stack pointer after all of the loads
      have been done.  */
   if (frame_reg_rtx != sp_reg_rtx)
     {
-      /* This blockage is needed so that sched doesn't decide to move
-	 the sp change before the register restores.  */
-      rs6000_emit_stack_tie ();
       if (TARGET_SPE_ABI
           && info->spe_64bit_regs_used != 0
           && info->first_gp_reg_save != 32)

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Andrew Pinski]

On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote:
> Tested on powerpc-none-linux-gnuspe with no regressions.  OK to commit?

I think this is needed for all sysv based abis and not just SPE.

Thanks,
Andrew Pinski

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Andrew Pinski]

On 10/3/07, Andrew Pinski <pinskia@gmail.com> wrote:
> On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > Tested on powerpc-none-linux-gnuspe with no regressions.  OK to commit?
>
> I think this is needed for all sysv based abis and not just SPE.

And I think this is related to PR30282.

-- Pinski

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Nathan Froyd]

On Wed, Oct 03, 2007 at 08:45:24AM -0700, Andrew Pinski wrote:
> On 10/3/07, Andrew Pinski <pinskia@gmail.com> wrote:
> > On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > > Tested on powerpc-none-linux-gnuspe with no regressions.  OK to commit?
> >
> > I think this is needed for all sysv based abis and not just SPE.
> 
> And I think this is related to PR30282.

Any objection to simply calling rs6000_emit_stack_tie unconditionally
before the stack adjustment, then?  (Patch below--I don't know what
AIX/Darwin requires here, but I can't imagine it would hurt performance
that much.)

-Nathan

Index: rs6000.c
===================================================================
--- rs6000.c	(revision 128981)
+++ rs6000.c	(working copy)
@@ -16329,13 +16329,14 @@ rs6000_emit_epilogue (int sibcall)
 	    }
     }
 
+  /* This blockage is needed so that sched doesn't decide to move
+     the sp change before the register restores.  */
+  rs6000_emit_stack_tie ();
+    
   /* If this is V.4, unwind the stack pointer after all of the loads
      have been done.  */
   if (frame_reg_rtx != sp_reg_rtx)
     {
-      /* This blockage is needed so that sched doesn't decide to move
-	 the sp change before the register restores.  */
-      rs6000_emit_stack_tie ();
       if (TARGET_SPE_ABI
           && info->spe_64bit_regs_used != 0
           && info->first_gp_reg_save != 32)

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Andrew Pinski]

On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote:
> Any objection to simply calling rs6000_emit_stack_tie unconditionally
> before the stack adjustment, then?  (Patch below--I don't know what
> AIX/Darwin requires here, but I can't imagine it would hurt performance
> that much.)
Inside rs6000_stack_info, we have the following comment:
  /* Determine if we need to allocate any stack frame:

     For AIX we need to push the stack if a frame pointer is needed
     (because the stack might be dynamically adjusted), if we are
     debugging, if we make calls, or if the sum of fp_save, gp_save,
     and local variables are more than the space needed to save all
     non-volatile registers: 32-bit: 18*8 + 19*4 = 220 or 64-bit: 18*8
     + 18*8 = 288 (GPR13 reserved).

     For V.4 we don't have the stack cushion that AIX uses, but assume
     that the debugger can handle stackless frames.  */

So only SVS V.4 ABI have this requirement which means PPC64-linux,
darwin or AIX have a red zone.  And yes this can worse code for OS's
with a red zone as the main reason why the movement is happening is so
the processor can handle the loads better.

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Daniel Jacobowitz]

On Wed, Oct 03, 2007 at 12:12:06PM -0700, Nathan Froyd wrote:
> On Wed, Oct 03, 2007 at 08:45:24AM -0700, Andrew Pinski wrote:
> > On 10/3/07, Andrew Pinski <pinskia@gmail.com> wrote:
> > > On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > > > Tested on powerpc-none-linux-gnuspe with no regressions.  OK to commit?
> > >
> > > I think this is needed for all sysv based abis and not just SPE.
> > 
> > And I think this is related to PR30282.
> 
> Any objection to simply calling rs6000_emit_stack_tie unconditionally
> before the stack adjustment, then?  (Patch below--I don't know what
> AIX/Darwin requires here, but I can't imagine it would hurt performance
> that much.)

Do some of these have a red zone?  If so, they don't need it.  I think
PPC64 Linux does.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Nathan Froyd]

On Wed, Oct 03, 2007 at 12:17:52PM -0700, Andrew Pinski wrote:
> On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > Any objection to simply calling rs6000_emit_stack_tie unconditionally
> > before the stack adjustment, then?  (Patch below--I don't know what
> > AIX/Darwin requires here, but I can't imagine it would hurt performance
> > that much.)
> Inside rs6000_stack_info, we have the following comment:
> 
> So only SVS V.4 ABI have this requirement which means PPC64-linux,
> darwin or AIX have a red zone.  And yes this can worse code for OS's
> with a red zone as the main reason why the movement is happening is so
> the processor can handle the loads better.

OK, thanks for the explanation.  So, modified patch below.

-Nathan

Index: rs6000.c
===================================================================
--- rs6000.c	(revision 128981)
+++ rs6000.c	(working copy)
@@ -16329,13 +16329,15 @@ rs6000_emit_epilogue (int sibcall)
 	    }
     }
 
+  /* This blockage is needed so that sched doesn't decide to move
+     the sp change before the register restores.  */
+  if (DEFAULT_ABI == ABI_V4)
+    rs6000_emit_stack_tie ();
+    
   /* If this is V.4, unwind the stack pointer after all of the loads
      have been done.  */
   if (frame_reg_rtx != sp_reg_rtx)
     {
-      /* This blockage is needed so that sched doesn't decide to move
-	 the sp change before the register restores.  */
-      rs6000_emit_stack_tie ();
       if (TARGET_SPE_ABI
           && info->spe_64bit_regs_used != 0
           && info->first_gp_reg_save != 32)

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Alan Modra]

On Wed, Oct 03, 2007 at 08:41:58AM -0700, Andrew Pinski wrote:
> On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > Tested on powerpc-none-linux-gnuspe with no regressions.  OK to commit?
> 
> I think this is needed for all sysv based abis and not just SPE.

No, it shouldn't be needed.  Plain powerpc-linux will restore regs
using either sp or fp relative memory addressing.  gcc shouldn't move
the sp adjust above use of sp, which is why we currently only emit the
stack tie when frame_reg_rtx != sp_reg_rtx.  The problem appears to be
specific to spe, which uses yet another register rtx to restore regs.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Alan Modra]

On Thu, Oct 04, 2007 at 11:12:53AM +0930, Alan Modra wrote:
> On Wed, Oct 03, 2007 at 08:41:58AM -0700, Andrew Pinski wrote:
> > I think this is needed for all sysv based abis and not just SPE.
> 
> No, it shouldn't be needed.  Plain powerpc-linux will restore regs
> using either sp or fp relative memory addressing.  gcc shouldn't move
> the sp adjust above use of sp, which is why we currently only emit the
> stack tie when frame_reg_rtx != sp_reg_rtx.  The problem appears to be
> specific to spe, which uses yet another register rtx to restore regs.

That wasn't a really clear explanation.  What I should have said was:

The scheduler will not move the stack adjustment (via sp_reg_rtx) past
register loads using a MEM with sp_reg_rtx in the address.  We only
need the stack tie when we reference MEMs without sp_reg_rtx in their
addresses.  If you examine rs6000_emit_epilogue you'll see that all of
the register restores use frame_reg_rtx in the MEM address, except for
some SPE code.  frame_reg_rtx is set to sp_reg_rtx at the start of
this function, and changed in only one place, for ABI_V4.  Thus, prior
to the addition of SPE support, it was correct to test
frame_reg_rtx != sp_reg_rtx to determine whether a stack tie was
needed.

So, another fix for the SPE problem is to use frame_reg_rtx in the SPE
support code instead of inventing another rtx for r11.  That's what I
set out to do in the following patch, but then noticed a few other
problems..

Firstly, there's a glaring error dating back to when Eric moved the
altivec register restore before the stack update.  Before we pop the
stack frame, sp_offset can't possibly depend on the size of the stack
frame, only on whether we have a frame or not.

Secondly, the SPE stack restore is wrong when
use_backchain_to_restore_sp and spe_regs_addressable_via_sp.  In that
case, r11 is not set by the SPE register restore code so remains
pointing to the top of the stack frame.

	* config/rs6000/rs6000.c (rs6000_emit_epilogue): Correct
	altivec sp_offset.  Rearrange sp_offset assignments to
	correspond to stack adjustments.  Use frame_reg_rtx for
	SPE register restores.  Correct SPE stack adjustment.

A powerpc-linux bootstrap/regtest is still in progress.  I don't have
access to SPE hardware to properly test the SPE changes, so would
someone mind regression testing this for me on SPE?

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 128777)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -15998,8 +16014,8 @@ rs6000_emit_epilogue (int sibcall)
       return;
     }
 
-  /* Set sp_offset based on the stack push from the prologue.  */
-  if (info->total_size < 32767)
+  /* frame_reg_rtx + sp_offset points to the top of this stack frame.  */
+  if (info->push_p)
     sp_offset = info->total_size;
 
   /* Restore AltiVec registers if needed.  */
@@ -16041,8 +16057,6 @@ rs6000_emit_epilogue (int sibcall)
       emit_insn (generate_set_vrsave (reg, info, 1));
     }
 
-  sp_offset = 0;
-
   /* 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.  */
@@ -16055,20 +16069,18 @@ rs6000_emit_epilogue (int sibcall)
 
       emit_move_insn (frame_reg_rtx,
 		      gen_rtx_MEM (Pmode, sp_reg_rtx));
+      sp_offset = 0;
     }
-  else if (info->push_p)
+  else if (info->push_p
+	   && DEFAULT_ABI != ABI_V4
+	   && !current_function_calls_eh_return)
     {
-      if (DEFAULT_ABI == ABI_V4
-	  || current_function_calls_eh_return)
-	sp_offset = info->total_size;
-      else
-	{
-	  emit_insn (TARGET_32BIT
-		     ? gen_addsi3 (sp_reg_rtx, sp_reg_rtx,
-				   GEN_INT (info->total_size))
-		     : gen_adddi3 (sp_reg_rtx, sp_reg_rtx,
-				   GEN_INT (info->total_size)));
-	}
+      emit_insn (TARGET_32BIT
+		 ? gen_addsi3 (sp_reg_rtx, sp_reg_rtx,
+			       GEN_INT (info->total_size))
+		 : gen_adddi3 (sp_reg_rtx, sp_reg_rtx,
+			       GEN_INT (info->total_size)));
+      sp_offset = 0;
     }
 
   /* Get the old lr if we saved it.  */
@@ -16150,7 +16162,6 @@ rs6000_emit_epilogue (int sibcall)
            && info->spe_64bit_regs_used != 0
            && info->first_gp_reg_save != 32)
     {
-      rtx spe_save_area_ptr;
       /* Determine whether we can address all of the registers that need
          to be saved with an offset from the stack pointer that fits in
          the small const field for SPE memory instructions.  */
@@ -16160,20 +16171,21 @@ rs6000_emit_epilogue (int sibcall)
       int spe_offset;
 
       if (spe_regs_addressable_via_sp)
-        {
-          spe_save_area_ptr = frame_reg_rtx;
-          spe_offset = info->spe_gp_save_offset + sp_offset;
-        }
+	spe_offset = info->spe_gp_save_offset + sp_offset;
       else
         {
+	  rtx old_frame_reg_rtx = frame_reg_rtx;
           /* Make r11 point to the start of the SPE save area.  We worried about
              not clobbering it when we were saving registers in the prologue.
              There's no need to worry here because the static chain is passed
              anew to every function.  */
-          spe_save_area_ptr = gen_rtx_REG (Pmode, 11);
-
-          emit_insn (gen_addsi3 (spe_save_area_ptr, frame_reg_rtx,
+	  if (frame_reg_rtx == sp_reg_rtx)
+	    frame_reg_rtx = gen_rtx_REG (Pmode, 11);
+          emit_insn (gen_addsi3 (frame_reg_rtx, old_frame_reg_rtx,
                                  GEN_INT (info->spe_gp_save_offset + sp_offset)));
+	  /* Keep the invariant that frame_reg_rtx + sp_offset points
+	     at the top of the stack frame.  */
+	  sp_offset = -info->spe_gp_save_offset;
 
           spe_offset = 0;
         }
@@ -16188,7 +16200,7 @@ rs6000_emit_epilogue (int sibcall)
             gcc_assert (SPE_CONST_OFFSET_OK (spe_offset + reg_size * i));
 
             offset = GEN_INT (spe_offset + reg_size * i);
-            addr = gen_rtx_PLUS (Pmode, spe_save_area_ptr, offset);
+            addr = gen_rtx_PLUS (Pmode, frame_reg_rtx, offset);
             mem = gen_rtx_MEM (V2SImode, addr);
 
             emit_move_insn (gen_rtx_REG (reg_mode, info->first_gp_reg_save + i),
@@ -16280,11 +16292,9 @@ rs6000_emit_epilogue (int sibcall)
       /* This blockage is needed so that sched doesn't decide to move
 	 the sp change before the register restores.  */
       rs6000_emit_stack_tie ();
-      if (TARGET_SPE_ABI
-          && info->spe_64bit_regs_used != 0
-          && info->first_gp_reg_save != 32)
-        emit_insn (gen_addsi3 (sp_reg_rtx, gen_rtx_REG (Pmode, 11),
-                               GEN_INT (-(info->spe_gp_save_offset + sp_offset))));
+      if (sp_offset != 0)
+        emit_insn (gen_addsi3 (sp_reg_rtx, frame_reg_rtx,
+			       GEN_INT (sp_offset)));
       else
         emit_move_insn (sp_reg_rtx, frame_reg_rtx);
     }

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Nathan Froyd]

On Fri, Oct 05, 2007 at 06:00:44PM +0930, Alan Modra wrote:
> So, another fix for the SPE problem is to use frame_reg_rtx in the SPE
> support code instead of inventing another rtx for r11.  That's what I
> set out to do in the following patch, but then noticed a few other
> problems..
> 
> 	* config/rs6000/rs6000.c (rs6000_emit_epilogue): Correct
> 	altivec sp_offset.  Rearrange sp_offset assignments to
> 	correspond to stack adjustments.  Use frame_reg_rtx for
> 	SPE register restores.  Correct SPE stack adjustment.
> 
> A powerpc-linux bootstrap/regtest is still in progress.  I don't have
> access to SPE hardware to properly test the SPE changes, so would
> someone mind regression testing this for me on SPE?

Bootstrap has proceeded without problems, regtesting underway.

-Nathan

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Nathan Froyd]

On Fri, Oct 05, 2007 at 08:02:31AM -0700, Nathan Froyd wrote:
> On Fri, Oct 05, 2007 at 06:00:44PM +0930, Alan Modra wrote:
> > A powerpc-linux bootstrap/regtest is still in progress.  I don't have
> > access to SPE hardware to properly test the SPE changes, so would
> > someone mind regression testing this for me on SPE?
> 
> Bootstrap has proceeded without problems, regtesting underway.

Regtesting complete, no regressions.

-Nathan

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[Alan Modra]

On Fri, Oct 05, 2007 at 06:00:44PM +0930, Alan Modra wrote:
> 	* config/rs6000/rs6000.c (rs6000_emit_epilogue): Correct
> 	altivec sp_offset.  Rearrange sp_offset assignments to
> 	correspond to stack adjustments.  Use frame_reg_rtx for
> 	SPE register restores.  Correct SPE stack adjustment.

Ping?  http://gcc.gnu.org/ml/gcc-patches/2007-10/msg00283.html

Besides the SPE issue, the patch also fixes restore of altivec regs in
large stack frames for !WORLD_SAVE_P ABIs.  Bootstrap and regression
testing has of course completed on powerpc-linux.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets

[David Edelsohn]

	* config/rs6000/rs6000.c (rs6000_emit_epilogue): Correct
	altivec sp_offset.  Rearrange sp_offset assignments to
	correspond to stack adjustments.  Use frame_reg_rtx for
	SPE register restores.  Correct SPE stack adjustment.


Okay.

Thanks, David