[rs6000] Fix PR 50906, eh_frame and other woes

Lots of bugs here.  Most of them in TARGET_SPE_ABI code, but some also
for other ABIs.

1) Marking an instruction setting up r11 for use by _save64gpr_* as
frame-related results in r11 being set as the cfa for the rest of the
function.  That's bad when r11 gets used for other things later.
Even worse, the offset given was wrong.  This bug was likely the
primary cause for the segfault reported in the PR.

2) RTL emitted to describe the _save64gpr_* used wrong offsets.
Harmless at the moment, I think, but still incorrect.  Ditto for the
epilogue _rest64gpr_* RTL.  eg. _save64gpr_24 rtl said the store for
r24 happened at 0(11), but see e500crtsav64gpr.S.

3) The .cfi_offset values given for the SPE _save64gpr_* reg saves
were wrong.

4) sp_offset was not set correctly in a number of places where
out-of-line save and restore functions were used, possibly leading to
incorrect code later in the prologue/epilogue.  For instance in this
code:

+	  emit_insn (gen_add3_insn (src_reg, frame_reg_rtx,
 				    GEN_INT (sp_offset - info->fp_size)));
-	  if (REGNO (frame_reg_rtx) == 11)
-	    sp_offset += info->fp_size;
+	  if (REGNO (frame_reg_rtx) == REGNO (src_reg))
+	    sp_offset = info->fp_size;

src_reg can be r12, so testing for r11 as frame_reg_rtx is wrong.  You
want to update sp_offset when frame_reg_rtx changes.  Also, adding
info->fp_size to sp_offset is obviously wrong when you've just added
(sp_offset - info->fp_size) to frame_reg_rtx.  Clearly you want to
subtract (sp_offset - info->fp_size) from sp_offset, to keep the
invariant that frame_reg_rtx + sp_offset points to top of frame.
Really, sp_offset should be renamed to frame_offset.

5) The wrong register mode was used in a number of places for address
calculations.  A nitpick, but reg_mode is for data, Pmode for addresses.

6) The epilogue _rest64gpr_* r11 wasn't always set up.

7) Not fixed by this patch, but many instances of rs6000_frame_related
and emit_frame_save pass frame_ptr_rtx for the reg to replace with
sp+offset in the cfi expression, when frame_ptr_rtx is not the reg
being used in the memory access.  That means we're relying on the
generic dwarf code tracking registers for us.  Which does seem to
work.  However, it's confusing to have calls to rs6000_frame_related
that really don't do anything.

Bootstrapped and regression tested powerpc-linux, with standard
BOOT_CFLAGS, with BOOT_CFLAGS="-g -Os" (tends to test just out-of-line
restore), and with BOOT_CFLAGS="-g -Os -mno-multiple" (test
out-of-line save and restore).  The PR reporter is running SPE
bootstrap and regression tests.  Incidentally, I had to use
--disable-werror as we run into some bogus warnings when trying to
bootstrap with -Os.

gcc/calls.c: In function 'rtx_def* expand_call(tree, rtx, int)':
gcc/calls.c:1098:52: warning: 'base' may be used uninitialized in this function [-Wmaybe-uninitialized]
gcc/calls.c:1079:9: note: 'base' was declared here

OK to apply?  gcc-4.6 branch too?

	PR target/50906
	* config/rs6000/rs6000.c (rs6000_emit_prologue <TARGET_SPE_ABI>):
	Do not mark r11 setup as frame-related.  Pass correct offset to
	rs6000_emit_savres_rtx.  Correct out-of-line rs6000_frame_related
	arguments.  Correct sp_offset.  Remove "offset" fudge from
	in-line rs6000_frame_related call.  Rename misleading variable.
	Fix comments and whitespace.  Tidy some expressions.
	(rs6000_emit_epilogue <TARGET_SPE_ABI>): Always set frame_reg_rtx
	to r11 in out-of-line case.  Correct sp_offset.  Pass correct
	offset to rs6000_emit_savres_rtx.  Rename misleading variable.
	Fix comments and whitespace.  Tidy some expressions.  
	(rs6000_emit_epilogue <non-TARGET_SPE_ABI>): Add sp_offset
	adjustment when !saving_GPRs_inline.  Correct register mode
	used in address calcs.
	(rs6000_emit_epilogue <non-TARGET_SPE_ABI>): Similarly when
	!restoring_GPRs_inline.

Hello Alan,

On 4 Nov 2011, at 13:07, Alan Modra wrote:

> Lots of bugs here.  Most of them in TARGET_SPE_ABI code, but some also
> for other ABIs.
>
> 1) Marking an instruction setting up r11 for use by _save64gpr_* as
> frame-related results in r11 being set as the cfa for the rest of the
> function.  That's bad when r11 gets used for other things later.
> Even worse, the offset given was wrong.  This bug was likely the
> primary cause for the segfault reported in the PR.
>
> 2) RTL emitted to describe the _save64gpr_* used wrong offsets.
> Harmless at the moment, I think, but still incorrect.  Ditto for the
> epilogue _rest64gpr_* RTL.  eg. _save64gpr_24 rtl said the store for
> r24 happened at 0(11), but see e500crtsav64gpr.S.
>
> 3) The .cfi_offset values given for the SPE _save64gpr_* reg saves
> were wrong.
>
> 4) sp_offset was not set correctly in a number of places where
> out-of-line save and restore functions were used, possibly leading to
> incorrect code later in the prologue/epilogue.  For instance in this
> code:
>
> +	  emit_insn (gen_add3_insn (src_reg, frame_reg_rtx,
> 				    GEN_INT (sp_offset - info->fp_size)));
> -	  if (REGNO (frame_reg_rtx) == 11)
> -	    sp_offset += info->fp_size;
> +	  if (REGNO (frame_reg_rtx) == REGNO (src_reg))
> +	    sp_offset = info->fp_size;
>
> src_reg can be r12, so testing for r11 as frame_reg_rtx is wrong.  You
> want to update sp_offset when frame_reg_rtx changes.  Also, adding
> info->fp_size to sp_offset is obviously wrong when you've just added
> (sp_offset - info->fp_size) to frame_reg_rtx.  Clearly you want to
> subtract (sp_offset - info->fp_size) from sp_offset, to keep the
> invariant that frame_reg_rtx + sp_offset points to top of frame.
> Really, sp_offset should be renamed to frame_offset.
>
> 5) The wrong register mode was used in a number of places for address
> calculations.  A nitpick, but reg_mode is for data, Pmode for  
> addresses.
>
> 6) The epilogue _rest64gpr_* r11 wasn't always set up.
>
> 7) Not fixed by this patch, but many instances of rs6000_frame_related
> and emit_frame_save pass frame_ptr_rtx for the reg to replace with
> sp+offset in the cfi expression, when frame_ptr_rtx is not the reg
> being used in the memory access.  That means we're relying on the
> generic dwarf code tracking registers for us.  Which does seem to
> work.  However, it's confusing to have calls to rs6000_frame_related
> that really don't do anything.
>
> Bootstrapped and regression tested powerpc-linux, with standard
> BOOT_CFLAGS, with BOOT_CFLAGS="-g -Os" (tends to test just out-of-line
> restore), and with BOOT_CFLAGS="-g -Os -mno-multiple" (test
> out-of-line save and restore).  The PR reporter is running SPE
> bootstrap and regression tests.  Incidentally, I had to use
> --disable-werror as we run into some bogus warnings when trying to
> bootstrap with -Os.

I did wonder, when implementing the out-of-line saves for Darwin,  
whether the setting in sysv4.h is a bit optimistic in firing for r31  
when -Os is on.

whether it saves space, in reality, would presumably depend on whether  
branch islands were inserted to reach the common code.

----

Bootstrapped this,  [together with http://gcc.gnu.org/ml/gcc-patches/2011-11/msg00550.html 
]
all+ada+java on powerpc-darwin9.  Tests started (slow machine .. will  
take a while)

Iain

> gcc/calls.c: In function 'rtx_def* expand_call(tree, rtx, int)':
> gcc/calls.c:1098:52: warning: 'base' may be used uninitialized in  
> this function [-Wmaybe-uninitialized]
> gcc/calls.c:1079:9: note: 'base' was declared here
>
> OK to apply?  gcc-4.6 branch too?
>
> 	PR target/50906
> 	* config/rs6000/rs6000.c (rs6000_emit_prologue <TARGET_SPE_ABI>):
> 	Do not mark r11 setup as frame-related.  Pass correct offset to
> 	rs6000_emit_savres_rtx.  Correct out-of-line rs6000_frame_related
> 	arguments.  Correct sp_offset.  Remove "offset" fudge from
> 	in-line rs6000_frame_related call.  Rename misleading variable.
> 	Fix comments and whitespace.  Tidy some expressions.
> 	(rs6000_emit_epilogue <TARGET_SPE_ABI>): Always set frame_reg_rtx
> 	to r11 in out-of-line case.  Correct sp_offset.  Pass correct
> 	offset to rs6000_emit_savres_rtx.  Rename misleading variable.
> 	Fix comments and whitespace.  Tidy some expressions.
> 	(rs6000_emit_epilogue <non-TARGET_SPE_ABI>): Add sp_offset
> 	adjustment when !saving_GPRs_inline.  Correct register mode
> 	used in address calcs.
> 	(rs6000_emit_epilogue <non-TARGET_SPE_ABI>): Similarly when
> 	!restoring_GPRs_inline.
>
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c	(revision 180867)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -20089,56 +20115,52 @@ rs6000_emit_prologue (void)
>     {
>       int i;
>       rtx spe_save_area_ptr;
> -
> +      int save_ptr_to_sp;
> +      int ool_adjust = 0;
> +
>       /* Determine whether we can address all of the registers that  
> need
> -	 to be saved with an offset from the stack pointer that fits in
> +	 to be saved with an offset from frame_reg_rtx that fits in
> 	 the small const field for SPE memory instructions.  */
> -      int spe_regs_addressable_via_sp
> -	= (SPE_CONST_OFFSET_OK(info->spe_gp_save_offset + sp_offset
> -			       + (32 - info->first_gp_reg_save - 1) * reg_size)
> +      int spe_regs_addressable
> +	= (SPE_CONST_OFFSET_OK (info->spe_gp_save_offset + sp_offset
> +				+ reg_size * (32 - info->first_gp_reg_save - 1))
> 	   && saving_GPRs_inline);
>       int spe_offset;
> -
> -      if (spe_regs_addressable_via_sp)
> +
> +      if (spe_regs_addressable)
> 	{
> 	  spe_save_area_ptr = frame_reg_rtx;
> +	  save_ptr_to_sp = info->total_size - sp_offset;
> 	  spe_offset = info->spe_gp_save_offset + sp_offset;
> 	}
>       else
> 	{
> 	  /* Make r11 point to the start of the SPE save area.  We need
> 	     to be careful here if r11 is holding the static chain.  If
> -	     it is, then temporarily save it in r0.  We would use r0 as
> -	     our base register here, but using r0 as a base register in
> -	     loads and stores means something different from what we
> -	     would like.  */
> -	  int ool_adjust = (saving_GPRs_inline
> -			    ? 0
> -			    : (info->first_gp_reg_save
> -			       - (FIRST_SAVRES_REGISTER+1))*8);
> -	  HOST_WIDE_INT offset = (info->spe_gp_save_offset
> -				  + sp_offset - ool_adjust);
> +	     it is, then temporarily save it in r0.  */
> +	  int offset;
> +
> +	  if (!saving_GPRs_inline)
> +	    ool_adjust = 8 * (info->first_gp_reg_save
> +			      - (FIRST_SAVRES_REGISTER + 1));
> +	  offset = info->spe_gp_save_offset + sp_offset - ool_adjust;
> +	  spe_save_area_ptr = gen_rtx_REG (Pmode, 11);
> +	  save_ptr_to_sp = info->total_size - sp_offset + offset;
> +	  spe_offset = 0;
>
> 	  if (using_static_chain_p)
> 	    {
> 	      rtx r0 = gen_rtx_REG (Pmode, 0);
> 	      gcc_assert (info->first_gp_reg_save > 11);
> -
> -	      emit_move_insn (r0, gen_rtx_REG (Pmode, 11));
> +
> +	      emit_move_insn (r0, spe_save_area_ptr);
> 	    }
> -
> -	  spe_save_area_ptr = gen_rtx_REG (Pmode, 11);
> -	  insn = emit_insn (gen_addsi3 (spe_save_area_ptr,
> -					frame_reg_rtx,
> -					GEN_INT (offset)));
> -	  /* We need to make sure the move to r11 gets noted for
> -	     properly outputting unwind information.  */
> -	  if (!saving_GPRs_inline)
> -	    rs6000_frame_related (insn, frame_reg_rtx, offset,
> -				  NULL_RTX, NULL_RTX);
> -	  spe_offset = 0;
> +	  emit_insn (gen_addsi3 (spe_save_area_ptr,
> +				 frame_reg_rtx, GEN_INT (offset)));
> +	  if (REGNO (frame_reg_rtx) == 11)
> +	    sp_offset = -info->spe_gp_save_offset + ool_adjust;
> 	}
> -
> +
>       if (saving_GPRs_inline)
> 	{
> 	  for (i = 0; i < 32 - info->first_gp_reg_save; i++)
> @@ -20150,58 +20172,65 @@ rs6000_emit_prologue (void)
> 		/* We're doing all this to ensure that the offset fits into
> 		   the immediate offset of 'evstdd'.  */
> 		gcc_assert (SPE_CONST_OFFSET_OK (reg_size * i + spe_offset));
> -
> +
> 		offset = GEN_INT (reg_size * i + spe_offset);
> 		addr = gen_rtx_PLUS (Pmode, spe_save_area_ptr, offset);
> 		mem = gen_rtx_MEM (V2SImode, addr);
> -
> +
> 		insn = emit_move_insn (mem, reg);
> -	
> -		rs6000_frame_related (insn, spe_save_area_ptr,
> -				      info->spe_gp_save_offset
> -				      + sp_offset + reg_size * i,
> -				      offset, const0_rtx);
> +
> +		rs6000_frame_related (insn,
> +				      spe_save_area_ptr, save_ptr_to_sp,
> +				      NULL_RTX, NULL_RTX);
> 	      }
> 	}
>       else
> 	{
> -	  insn = rs6000_emit_savres_rtx (info, gen_rtx_REG (Pmode, 11),
> -					 0, reg_mode,
> +	  insn = rs6000_emit_savres_rtx (info, spe_save_area_ptr,
> +					 ool_adjust, reg_mode,
> 					 /*savep=*/true, /*gpr=*/true,
> 					 /*lr=*/false);
> -	  rs6000_frame_related (insn, frame_ptr_rtx, info->total_size,
> +
> +	  rs6000_frame_related (insn, spe_save_area_ptr, save_ptr_to_sp,
> 				NULL_RTX, NULL_RTX);
> 	}
> -					
> -
> +
>       /* Move the static chain pointer back.  */
> -      if (using_static_chain_p && !spe_regs_addressable_via_sp)
> -	emit_move_insn (gen_rtx_REG (Pmode, 11), gen_rtx_REG (Pmode, 0));
> +      if (using_static_chain_p && !spe_regs_addressable)
> +	emit_move_insn (spe_save_area_ptr, gen_rtx_REG (Pmode, 0));
>     }
>   else if (!WORLD_SAVE_P (info) && !saving_GPRs_inline)
>     {
>       if (DEFAULT_ABI == ABI_DARWIN)
> 	{
> -	  rtx dest_reg = gen_rtx_REG (reg_mode, 11);
> +	  rtx dest_reg = gen_rtx_REG (Pmode, 11);
> 	  if (info->first_fp_reg_save == 64)
> -	    /* we only need a copy, no fprs were saved.  */
> -	    emit_move_insn (dest_reg, frame_reg_rtx);
> +	    {
> +	      /* we only need a copy, no fprs were saved.  */
> +	      if (dest_reg != frame_reg_rtx)
> +		emit_move_insn (dest_reg, frame_reg_rtx);
> +	    }
> 	  else
> 	    {
> -	      rtx offset = GEN_INT (sp_offset
> -				    + (-8 * (64-info->first_fp_reg_save)));
> +	      int save_off = 8 * (64 - info->first_fp_reg_save);
> +	      rtx offset = GEN_INT (sp_offset - save_off);
> +
> +	      if (REGNO (dest_reg) == REGNO (frame_reg_rtx))
> +		sp_offset = save_off;
> 	      emit_insn (gen_add3_insn (dest_reg, frame_reg_rtx, offset));
> 	    }
> 	}
>       /* Need to adjust r11 (r12) if we saved any FPRs.  */
>       else if (info->first_fp_reg_save != 64)
> -        {
> -	  rtx dest_reg = gen_rtx_REG (reg_mode, DEFAULT_ABI == ABI_AIX
> -				      ? 12 : 11);
> -	  rtx offset = GEN_INT (sp_offset
> -                                + (-8 * (64-info- 
> >first_fp_reg_save)));
> +	{
> +	  rtx dest_reg = gen_rtx_REG (Pmode, DEFAULT_ABI == ABI_AIX ? 12 :  
> 11);
> +	  int save_off = 8 * (64 - info->first_fp_reg_save);
> +	  rtx offset = GEN_INT (sp_offset - save_off);
> +
> +	  if (REGNO (dest_reg) == REGNO (frame_reg_rtx))
> +	    sp_offset = save_off;
> 	  emit_insn (gen_add3_insn (dest_reg, frame_reg_rtx, offset));
> -        }
> +	}
>
>       insn = rs6000_emit_savres_rtx (info, frame_reg_rtx,
> 				     info->gp_save_offset + sp_offset,
> @@ -21115,40 +21162,39 @@ rs6000_emit_epilogue (int sibcall)
>       && info->first_gp_reg_save != 32)
>     {
>       /* 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.  */
> -      int spe_regs_addressable_via_sp
> -	= (SPE_CONST_OFFSET_OK(info->spe_gp_save_offset + sp_offset
> -			       + (32 - info->first_gp_reg_save - 1) * reg_size)
> +	 to be saved with an offset from frame_reg_rtx that fits in
> +	 the small const field for SPE memory instructions.  */
> +      int spe_regs_addressable
> +	= (SPE_CONST_OFFSET_OK (info->spe_gp_save_offset + sp_offset
> +				+ reg_size * (32 - info->first_gp_reg_save - 1))
> 	   && restoring_GPRs_inline);
>       int spe_offset;
> +      int ool_adjust = 0;
>
> -      if (spe_regs_addressable_via_sp)
> +      if (spe_regs_addressable)
> 	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.  */
> -	  int ool_adjust = (restoring_GPRs_inline
> -			    ? 0
> -			    : (info->first_gp_reg_save
> -			       - (FIRST_SAVRES_REGISTER + 1)) * 8);
> -
> -	  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,
> +	  /* 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.  */
> +
> +	  if (!restoring_GPRs_inline)
> +	    ool_adjust = 8 * (info->first_gp_reg_save
> +			      - (FIRST_SAVRES_REGISTER + 1));
> +	  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
> 					  - ool_adjust)));
> 	  /* Keep the invariant that frame_reg_rtx + sp_offset points
> 	     at the top of the stack frame.  */
> -	  sp_offset = -info->spe_gp_save_offset;
> +	  sp_offset = -info->spe_gp_save_offset + ool_adjust;
>
> -          spe_offset = 0;
> -        }
> +	  spe_offset = 0;
> +	}
>
>       if (restoring_GPRs_inline)
> 	{
> @@ -21170,8 +21216,8 @@ rs6000_emit_epilogue (int sibcall)
> 	      }
> 	}
>       else
> -	rs6000_emit_savres_rtx (info, gen_rtx_REG (Pmode, 11),
> -				0, reg_mode,
> +	rs6000_emit_savres_rtx (info, frame_reg_rtx,
> +				ool_adjust, reg_mode,
> 				/*savep=*/false, /*gpr=*/true,
> 				/*lr=*/true);
>     }
> @@ -21184,22 +21230,22 @@ rs6000_emit_epilogue (int sibcall)
>       if (can_use_exit)
> 	{
> 	  rs6000_emit_stack_reset (info, sp_reg_rtx, frame_reg_rtx,
> -				 sp_offset, can_use_exit);
> +				   sp_offset, can_use_exit);
> 	  if (DEFAULT_ABI == ABI_DARWIN)
> 	    /* we only need a copy, no fprs were saved.  */
> -	    emit_move_insn (gen_rtx_REG (reg_mode, 11), frame_reg_rtx);
> +	    emit_move_insn (gen_rtx_REG (Pmode, 11), frame_reg_rtx);
>
> 	  if (info->cr_save_p)
> 	    rs6000_restore_saved_cr (cr_save_reg, using_mtcr_multiple);
> 	}
>       else
> 	{
> -	  emit_insn (gen_add3_insn (gen_rtx_REG (Pmode, DEFAULT_ABI ==  
> ABI_AIX
> -							? 12 : 11),
> -				    frame_reg_rtx,
> +	  rtx src_reg = gen_rtx_REG (Pmode, DEFAULT_ABI == ABI_AIX ? 12 :  
> 11);
> +
> +	  emit_insn (gen_add3_insn (src_reg, frame_reg_rtx,
> 				    GEN_INT (sp_offset - info->fp_size)));
> -	  if (REGNO (frame_reg_rtx) == 11)
> -	    sp_offset += info->fp_size;
> +	  if (REGNO (frame_reg_rtx) == REGNO (src_reg))
> +	    sp_offset = info->fp_size;
> 	}
>
>       rs6000_emit_savres_rtx (info, frame_reg_rtx,
>
> -- 
> Alan Modra
> Australia Development Lab, IBM

On Fri, Nov 04, 2011 at 06:19:07PM +0000, Iain Sandoe wrote:
> I did wonder, when implementing the out-of-line saves for Darwin,
> whether the setting in sysv4.h is a bit optimistic in firing for r31
> when -Os is on.
> 
> whether it saves space, in reality, would presumably depend on
> whether branch islands were inserted to reach the common code.

Yes, but that doesn't happen on powerpc-linux until code size goes
above 33M.  We get a (small) gain otherwise when able to use the
exit versions of the out-of-line restores.

> Bootstrapped this,

Thanks!

On Nov 4, 2011, at 2:07 PM, Alan Modra wrote:

> Lots of bugs here.  Most of them in TARGET_SPE_ABI code, but some also
> for other ABIs.

...

Another bug we're running into here is an unwarranted move of the sp
restore prior to register fetches despite an attempt at preventing that
with a stack_tie instruction (VxWorks target).

  http://gcc.gnu.org/ml/gcc/2011-03/msg00123.html

The failure can still be exposed on mainline with a minor adjustment
to the C testcase quoted in the msg.

I have been looking into this issue again recently and I'm convinced
that the the current stack_tie insn just is not quite adequate.

I have a local patch which fixes the case at hand but I'm
still unclear about a couple of aspects.

There are lots of subtle inter-section dependencies and redundancies
in emit_epilogue, which has grown pretty difficult to understand IMHO.

I can see two tracks to improve things in this area:

- Concentrate on the sp-move problem at hand and get to an agreement
  on the proper correction for it. I'll be happy to send my current
  patch with comments and questions for that.

- Start working on reducing emit_epilogue's complexity, e.g. by first
  extracting portions of it out into separate functions, then see what
  concepts we can identify and materialize to simplify the sequence
  organization as a whole.
 
  I did an extraction attempt as an exercise to better understand the
  various sections and dependencies. It did help me and I'd be happy
  to provide patches gather feedback etc if you believe it might be
  useful to others.

Olivier

On 7 Nov 2011, at 09:53, Olivier Hainque wrote:

> On Nov 4, 2011, at 2:07 PM, Alan Modra wrote:
>
>> Lots of bugs here.  Most of them in TARGET_SPE_ABI code, but some  
>> also
>> for other ABIs.
>
> ...
>
> Another bug we're running into here is an unwarranted move of the sp
> restore prior to register fetches despite an attempt at preventing  
> that
> with a stack_tie instruction (VxWorks target).
>
>  http://gcc.gnu.org/ml/gcc/2011-03/msg00123.html
>
> The failure can still be exposed on mainline with a minor adjustment
> to the C testcase quoted in the msg.
>
> I have been looking into this issue again recently and I'm convinced
> that the the current stack_tie insn just is not quite adequate.
>
> I have a local patch which fixes the case at hand but I'm
> still unclear about a couple of aspects.
>
> There are lots of subtle inter-section dependencies and redundancies
> in emit_epilogue, which has grown pretty difficult to understand IMHO.
>
> I can see two tracks to improve things in this area:
>
> - Concentrate on the sp-move problem at hand and get to an agreement
>  on the proper correction for it. I'll be happy to send my current
>  patch with comments and questions for that.
>
> - Start working on reducing emit_epilogue's complexity, e.g. by first
>  extracting portions of it out into separate functions, then see what
>  concepts we can identify and materialize to simplify the sequence
>  organization as a whole.

I'd welcome this too - I was considering partitioning out the  
save_world () stuff - since it's only used by Darwin AFAICT.

Iain

>  I did an extraction attempt as an exercise to better understand the
>  various sections and dependencies. It did help me and I'd be happy
>  to provide patches gather feedback etc if you believe it might be
>  useful to others.
>
> Olivier
>
>
>
>

On Mon, Nov 07, 2011 at 10:53:51AM +0100, Olivier Hainque wrote:
> Another bug we're running into here is an unwarranted move of the sp
> restore prior to register fetches despite an attempt at preventing that
> with a stack_tie instruction (VxWorks target).
> 
>   http://gcc.gnu.org/ml/gcc/2011-03/msg00123.html
> 
> The failure can still be exposed on mainline with a minor adjustment
> to the C testcase quoted in the msg.

Even after revision 181056?

On Nov 7, 2011, at 12:02 PM, Alan Modra wrote:

>> The failure can still be exposed on mainline with a minor adjustment
>> to the C testcase quoted in the msg.
> 
> Even after revision 181056?

 I'll double check but I'm pretty sure that this change cannot
 help the specific problem at hand. VxWorks is V4 and we do have
 a tie insn emitted. It's just that the tie fails to prevent the
 move. I'll re-check with the current sources and followup.

 Olivier

On Mon, Nov 7, 2011 at 4:53 AM, Olivier Hainque <hainque@adacore.com> wrote:

> There are lots of subtle inter-section dependencies and redundancies
> in emit_epilogue, which has grown pretty difficult to understand IMHO.
>
> I can see two tracks to improve things in this area:
>
> - Concentrate on the sp-move problem at hand and get to an agreement
>  on the proper correction for it. I'll be happy to send my current
>  patch with comments and questions for that.
>
> - Start working on reducing emit_epilogue's complexity, e.g. by first
>  extracting portions of it out into separate functions, then see what
>  concepts we can identify and materialize to simplify the sequence
>  organization as a whole.
>
>  I did an extraction attempt as an exercise to better understand the
>  various sections and dependencies. It did help me and I'd be happy
>  to provide patches gather feedback etc if you believe it might be
>  useful to others.

Refactoring the prologue and epilogue code in rs6000.c would be great
and welcomed.

Thanks, David

Ping http://gcc.gnu.org/ml/gcc-patches/2011-11/msg00543.html

On Fri, Nov 4, 2011 at 9:07 AM, Alan Modra <amodra@gmail.com> wrote:

> OK to apply?  gcc-4.6 branch too?
>
>        PR target/50906
>        * config/rs6000/rs6000.c (rs6000_emit_prologue <TARGET_SPE_ABI>):
>        Do not mark r11 setup as frame-related.  Pass correct offset to
>        rs6000_emit_savres_rtx.  Correct out-of-line rs6000_frame_related
>        arguments.  Correct sp_offset.  Remove "offset" fudge from
>        in-line rs6000_frame_related call.  Rename misleading variable.
>        Fix comments and whitespace.  Tidy some expressions.
>        (rs6000_emit_epilogue <TARGET_SPE_ABI>): Always set frame_reg_rtx
>        to r11 in out-of-line case.  Correct sp_offset.  Pass correct
>        offset to rs6000_emit_savres_rtx.  Rename misleading variable.
>        Fix comments and whitespace.  Tidy some expressions.
>        (rs6000_emit_epilogue <non-TARGET_SPE_ABI>): Add sp_offset
>        adjustment when !saving_GPRs_inline.  Correct register mode
>        used in address calcs.
>        (rs6000_emit_epilogue <non-TARGET_SPE_ABI>): Similarly when
>        !restoring_GPRs_inline.

Okay for trunk and 4.6.

Thanks, David