[RS6000] PR 80938, Don't emit eh_frame for regs that don't need saving

Message ID 20170811031011.GO16312@bubble.grove.modra.org
State New
Headers show

Commit Message

Alan Modra Aug. 11, 2017, 3:10 a.m.
It is possible when using out-of-line register saves or store multiple
to save some registers unnecessarily, for example one reg in the block
saved might be unused.  We don't need to emit eh_frame info for those
registers as that just bloats the eh_frame info, and also can result
in an ICE when shrink-wrap gives multiple paths through the function
saving different sets of registers.  All the join points need to have
identical eh_frame register save state.

This patch reverts the previous fix for PR80939 "Use SAVE_MULTIPLE
only if we restore what it saves (PR80938)" and instead fixes the PR
by correcting the eh_frame info.  The change to rs6000_savres_strategy
is an optimization, but note that it hides the underlying problem in
the PR testcase.

Bootstrapped and regression tested powerpc64-linux (-m32 too) and
powerpc64le-linux, with
https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00774.html and
https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00775.html applied.
OK to apply?

	PR target/80938
	* config/rs6000/rs6000.c (rs6000_savres_strategy): Revert 2017-08-09.
	Don't use store multiple if only one reg needs saving.
	(rs6000_frame_related): Don't emit eh_frame for regs that
	don't need saving.
	(rs6000_emit_epilogue): Likewise.

Comments

Segher Boessenkool Aug. 15, 2017, 7:28 a.m. | #1
Hi!

On Fri, Aug 11, 2017 at 12:40:11PM +0930, Alan Modra wrote:
> It is possible when using out-of-line register saves or store multiple
> to save some registers unnecessarily, for example one reg in the block
> saved might be unused.  We don't need to emit eh_frame info for those
> registers as that just bloats the eh_frame info, and also can result
> in an ICE when shrink-wrap gives multiple paths through the function
> saving different sets of registers.  All the join points need to have
> identical eh_frame register save state.

It isn't just eh_frame, we might have never noticed if it was.  It is
also the DWARF call frame info used for debugging.

> 	PR target/80938
> 	* config/rs6000/rs6000.c (rs6000_savres_strategy): Revert 2017-08-09.
> 	Don't use store multiple if only one reg needs saving.
> 	(rs6000_frame_related): Don't emit eh_frame for regs that
> 	don't need saving.
> 	(rs6000_emit_epilogue): Likewise.

> +      int count;
> +
> +      for (count = 0, i = info->first_gp_reg_save; i < 32; i++)
> +	if (save_reg_p (i))
> +	  count += 1;

      int count = 0;
      for (i = info->first_gp_reg_save; i < 32; i++)
	if (save_reg_p (i))
	  count++;

> @@ -25681,9 +25683,15 @@ rs6000_frame_related (rtx_insn *insn, rtx reg, HOST_WIDE_INT val,
>  		 register save functions, or store multiple, then omit
>  		 eh_frame info for any user-defined global regs.  If
>  		 eh_frame info is supplied, frame unwinding will
> -		 restore a user reg.  */
> +		 restore a user reg.  Also omit eh_frame info for any
> +		 reg we don't need to save, as that bloats eh_frame
> +		 and can cause problems with shrink wrapping.  Saves
> +		 of r0 are actually saving LR, so don't omit those.   */
>  	      if (!REG_P (SET_SRC (set))
> -		  || !fixed_reg_p (REGNO (SET_SRC (set))))
> +		  || REGNO (SET_SRC (set)) == 0
> +		  || REGNO (SET_SRC (set)) == CR2_REGNO
> +		  || (!fixed_reg_p (REGNO (SET_SRC (set)))
> +		      && save_reg_p (REGNO (SET_SRC (set)))))
>  		RTX_FRAME_RELATED_P (set) = 1;
>  	    }
>        RTX_FRAME_RELATED_P (insn) = 1;

That "0" and "CR2_REGNO" is non-obvious, it needs a big fat comment (it's
not clear that just CR2_REGNO is correct, either).  Oh, and please factor
out "REGNO (SET_SRC (set))".

The rest looks good, but please repost.


Segher

Patch

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 2070648..abc55bd 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -24432,20 +24432,37 @@  rs6000_savres_strategy (rs6000_stack_t *info,
 	   && flag_shrink_wrap_separate
 	   && optimize_function_for_speed_p (cfun)))
     {
-      /* Prefer store multiple for saves over out-of-line routines,
-	 since the store-multiple instruction will always be smaller.  */
-      strategy |= SAVE_INLINE_GPRS | SAVE_MULTIPLE;
-
-      /* The situation is more complicated with load multiple.  We'd
-	 prefer to use the out-of-line routines for restores, since the
-	 "exit" out-of-line routines can handle the restore of LR and the
-	 frame teardown.  However if doesn't make sense to use the
-	 out-of-line routine if that is the only reason we'd need to save
-	 LR, and we can't use the "exit" out-of-line gpr restore if we
-	 have saved some fprs; In those cases it is advantageous to use
-	 load multiple when available.  */
-      if (info->first_fp_reg_save != 64 || !lr_save_p)
-	strategy |= REST_INLINE_GPRS | REST_MULTIPLE;
+      int count;
+
+      for (count = 0, i = info->first_gp_reg_save; i < 32; i++)
+	if (save_reg_p (i))
+	  count += 1;
+
+      if (count <= 1)
+	/* Don't use store multiple if only one reg needs to be
+	   saved.  This can occur for example when the ABI_V4 pic reg
+	   (r30) needs to be saved to make calls, but r31 is not
+	   used.  */
+	strategy |= SAVE_INLINE_GPRS | REST_INLINE_GPRS;
+      else
+	{
+	  /* Prefer store multiple for saves over out-of-line
+	     routines, since the store-multiple instruction will
+	     always be smaller.  */
+	  strategy |= SAVE_INLINE_GPRS | SAVE_MULTIPLE;
+
+	  /* The situation is more complicated with load multiple.
+	     We'd prefer to use the out-of-line routines for restores,
+	     since the "exit" out-of-line routines can handle the
+	     restore of LR and the frame teardown.  However if doesn't
+	     make sense to use the out-of-line routine if that is the
+	     only reason we'd need to save LR, and we can't use the
+	     "exit" out-of-line gpr restore if we have saved some
+	     fprs; In those cases it is advantageous to use load
+	     multiple when available.  */
+	  if (info->first_fp_reg_save != 64 || !lr_save_p)
+	    strategy |= REST_INLINE_GPRS | REST_MULTIPLE;
+	}
     }
 
   /* Using the "exit" out-of-line routine does not improve code size
@@ -24454,21 +24471,6 @@  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;
-	  }
-    }
-
   /* Don't ever restore fixed regs.  */
   if ((strategy & (REST_INLINE_GPRS | REST_MULTIPLE)) != REST_INLINE_GPRS)
     for (i = info->first_gp_reg_save; i < 32; i++)
@@ -25681,9 +25683,15 @@  rs6000_frame_related (rtx_insn *insn, rtx reg, HOST_WIDE_INT val,
 		 register save functions, or store multiple, then omit
 		 eh_frame info for any user-defined global regs.  If
 		 eh_frame info is supplied, frame unwinding will
-		 restore a user reg.  */
+		 restore a user reg.  Also omit eh_frame info for any
+		 reg we don't need to save, as that bloats eh_frame
+		 and can cause problems with shrink wrapping.  Saves
+		 of r0 are actually saving LR, so don't omit those.   */
 	      if (!REG_P (SET_SRC (set))
-		  || !fixed_reg_p (REGNO (SET_SRC (set))))
+		  || REGNO (SET_SRC (set)) == 0
+		  || REGNO (SET_SRC (set)) == CR2_REGNO
+		  || (!fixed_reg_p (REGNO (SET_SRC (set)))
+		      && save_reg_p (REGNO (SET_SRC (set)))))
 		RTX_FRAME_RELATED_P (set) = 1;
 	    }
       RTX_FRAME_RELATED_P (insn) = 1;
@@ -25720,9 +25728,13 @@  rs6000_frame_related (rtx_insn *insn, rtx reg, HOST_WIDE_INT val,
 	      set = simplify_replace_rtx (set, reg2, repl2);
 	    XVECEXP (pat, 0, i) = set;
 
-	    /* Omit eh_frame info for any user-defined global regs.  */
+	    /* Omit eh_frame info for any user-defined global regs or
+	       regs that don't need to be saved.  */
 	    if (!REG_P (SET_SRC (set))
-		|| !fixed_reg_p (REGNO (SET_SRC (set))))
+		|| REGNO (SET_SRC (set)) == 0
+		|| REGNO (SET_SRC (set)) == CR2_REGNO
+		|| (!fixed_reg_p (REGNO (SET_SRC (set)))
+		    && save_reg_p (REGNO (SET_SRC (set)))))
 	      RTX_FRAME_RELATED_P (set) = 1;
 	  }
     }
@@ -27945,7 +27957,8 @@  rs6000_emit_epilogue (int sibcall)
 	  RTVEC_ELT (p, j++)
 	    = gen_frame_load (reg,
 			      frame_reg_rtx, info->gp_save_offset + reg_size * i);
-	  if (flag_shrink_wrap)
+	  if (flag_shrink_wrap
+	      && save_reg_p (info->first_gp_reg_save + i))
 	    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
 	}
       for (i = 0; info->first_altivec_reg_save + i <= LAST_ALTIVEC_REGNO; i++)
@@ -27954,7 +27967,8 @@  rs6000_emit_epilogue (int sibcall)
 	  RTVEC_ELT (p, j++)
 	    = gen_frame_load (reg,
 			      frame_reg_rtx, info->altivec_save_offset + 16 * i);
-	  if (flag_shrink_wrap)
+	  if (flag_shrink_wrap
+	      && save_reg_p (info->first_altivec_reg_save + i))
 	    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
 	}
       for (i = 0; info->first_fp_reg_save + i <= 63; i++)
@@ -27964,7 +27978,8 @@  rs6000_emit_epilogue (int sibcall)
 				 info->first_fp_reg_save + i);
 	  RTVEC_ELT (p, j++)
 	    = gen_frame_load (reg, frame_reg_rtx, info->fp_save_offset + 8 * i);
-	  if (flag_shrink_wrap)
+	  if (flag_shrink_wrap
+	      && save_reg_p (info->first_fp_reg_save + i))
 	    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
 	}
       RTVEC_ELT (p, j++)
@@ -28085,7 +28100,8 @@  rs6000_emit_epilogue (int sibcall)
 	    && (flag_shrink_wrap
 		|| (offset_below_red_zone_p
 		    (info->altivec_save_offset
-		     + 16 * (i - info->first_altivec_reg_save)))))
+		     + 16 * (i - info->first_altivec_reg_save))))
+	    && save_reg_p (i))
 	  {
 	    rtx reg = gen_rtx_REG (V4SImode, i);
 	    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
@@ -28297,7 +28313,8 @@  rs6000_emit_epilogue (int sibcall)
       for (i = info->first_altivec_reg_save; i <= LAST_ALTIVEC_REGNO; ++i)
 	if (((strategy & REST_INLINE_VRS) == 0
 	     || (info->vrsave_mask & ALTIVEC_REG_BIT (i)) != 0)
-	    && (DEFAULT_ABI == ABI_V4 || flag_shrink_wrap))
+	    && (DEFAULT_ABI == ABI_V4 || flag_shrink_wrap)
+	    && save_reg_p (i))
 	  {
 	    rtx reg = gen_rtx_REG (V4SImode, i);
 	    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
@@ -28643,7 +28660,8 @@  rs6000_emit_epilogue (int sibcall)
 
 	  RTVEC_ELT (p, elt++)
 	    = gen_frame_load (reg, sp_reg_rtx, info->fp_save_offset + 8 * i);
-	  if (flag_shrink_wrap)
+	  if (flag_shrink_wrap
+	      && save_reg_p (info->first_fp_reg_save + i))
 	    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
 	}