diff mbox

[RS6000] Rewrite rs6000_frame_related to use simplify_replace_rtx

Message ID 20160504014441.GR18915@bubble.grove.modra.org
State New
Headers show

Commit Message

Alan Modra May 4, 2016, 1:44 a.m. UTC 
was Re: [PATCH] Change replace_rtx if from is a REG (PR target/70245, take 2)
On Thu, Mar 17, 2016 at 11:07:03PM +1030, Alan Modra wrote:
> By the look of what you posted in the bugzilla, the pattern is the
> parallel emitted by rs6000_emit_savres_rtx.  In that parallel, the
> stack memory locations for register saves are described relative to
> whatever frame_reg_rtx is in use, which may be r12.
> rs6000_frame_related wants to translate the frame_reg_rtx into stack
> pointer plus offset for debug info.
> 
> The parallel matches save_gpregs_<mode>_r12 and similar in rs6000.md,
> which emit a call to an out-of-line register save function.  This
> function actually takes r12 as a parameter, hence the (use (reg:P 12))
> in the pattern.
> 
> rs6000_frame_related probably should just be replacing individual
> SETs in the parallel using simplify_replace_rtx.  Especially since
> after calling replace_rtx, it already iterates over them to simplify.

Like this.  Bootstrapped and regression tested powerpc64le-linux
and powerpc64-linux.

Modify SETs rather than using replace_rtx on the whole insn.
Removes fragile hacks preventing USE and CLOBBER being modified.

	* config/rs6000/rs6000.c (rs6000_frame_related): Rewrite.

Comments

Segher Boessenkool May 4, 2016, 4:26 p.m. UTC | #1 
On Wed, May 04, 2016 at 11:14:41AM +0930, Alan Modra wrote:
> 	* config/rs6000/rs6000.c (rs6000_frame_related): Rewrite.

> -  rtx real, temp;
> +  rtx patt, repl;

If you don't rename "real" here it is probably easier to read?  And it's
a better name anyway?

> -  if (REGNO (reg) == STACK_POINTER_REGNUM && reg2 == NULL_RTX)
> +  repl = NULL_RTX;
> +  if (REGNO (reg) == STACK_POINTER_REGNUM)
> +    gcc_checking_assert (val == 0);
> +  else
> +    repl = gen_rtx_PLUS (Pmode, gen_rtx_REG (Pmode, STACK_POINTER_REGNUM),
> +			 GEN_INT (val));

Put the NULL_RTX assignment in the first arm, please.

Okay for trunk with those changes, thanks,


Segher
Alan Modra May 4, 2016, 9:19 p.m. UTC | #2 
On Wed, May 04, 2016 at 11:26:18AM -0500, Segher Boessenkool wrote:
> On Wed, May 04, 2016 at 11:14:41AM +0930, Alan Modra wrote:
> > 	* config/rs6000/rs6000.c (rs6000_frame_related): Rewrite.
> 
> > -  rtx real, temp;
> > +  rtx patt, repl;
> 
> If you don't rename "real" here it is probably easier to read?

Easier to read the diff, yes.

>  And it's a better name anyway?

No, "real" seems silly to me.  "patt" is a common idiom used in lots
of places for the pattern of an instruction.  What is "real" supposed
to mean?  The real pattern vs. some imaginary one?  The final pattern
we want?  The last meaning might have made some sense in the very
first implementation of rs6000_frame_related where the code did
something like
  real = replace_rtx (PATTERN (insn), ...);
then made simplify_rtx calls.

I think "real" is confusing when we're making substitutions step by
step.

> > -  if (REGNO (reg) == STACK_POINTER_REGNUM && reg2 == NULL_RTX)
> > +  repl = NULL_RTX;
> > +  if (REGNO (reg) == STACK_POINTER_REGNUM)
> > +    gcc_checking_assert (val == 0);
> > +  else
> > +    repl = gen_rtx_PLUS (Pmode, gen_rtx_REG (Pmode, STACK_POINTER_REGNUM),
> > +			 GEN_INT (val));
> 
> Put the NULL_RTX assignment in the first arm, please.

OK, I'll make that style change, but only because we have that
gcc_checking_assert there.

Otherwise I would have written
  repl = NULL_RTX;
  if (REGNO (reg) != STACK_POINTER_REGNUM)
    repl = gen_rtx_PLUS (Pmode, gen_rtx_REG (Pmode, STACK_POINTER_REGNUM),
			 GEN_INT (val));

which is better than
  if (REGNO (reg) == STACK_POINTER_REGNUM)
    repl = NULL_RTX;
  else
    repl = gen_rtx_PLUS (Pmode, gen_rtx_REG (Pmode, STACK_POINTER_REGNUM),
			 GEN_INT (val));
Segher Boessenkool May 4, 2016, 11:03 p.m. UTC | #3 
On Thu, May 05, 2016 at 06:49:04AM +0930, Alan Modra wrote:
> >  And it's a better name anyway?
> 
> No, "real" seems silly to me.  "patt" is a common idiom used in lots
> of places for the pattern of an instruction.

"patt" is used only once (in fwprop), everything else uses "pat".

> What is "real" supposed
> to mean?  The real pattern vs. some imaginary one?

Yes exactly.  This function is making a note that does the same thing
as the real insn.

> The final pattern
> we want?  The last meaning might have made some sense in the very
> first implementation of rs6000_frame_related where the code did
> something like
>   real = replace_rtx (PATTERN (insn), ...);
> then made simplify_rtx calls.
> 
> I think "real" is confusing when we're making substitutions step by
> step.

True enough, okay, the later part of the function changes "real".

> > > -  if (REGNO (reg) == STACK_POINTER_REGNUM && reg2 == NULL_RTX)
> > > +  repl = NULL_RTX;
> > > +  if (REGNO (reg) == STACK_POINTER_REGNUM)
> > > +    gcc_checking_assert (val == 0);
> > > +  else
> > > +    repl = gen_rtx_PLUS (Pmode, gen_rtx_REG (Pmode, STACK_POINTER_REGNUM),
> > > +			 GEN_INT (val));
> > 
> > Put the NULL_RTX assignment in the first arm, please.
> 
> OK, I'll make that style change, but only because we have that
> gcc_checking_assert there.
> 
> Otherwise I would have written
>   repl = NULL_RTX;
>   if (REGNO (reg) != STACK_POINTER_REGNUM)
>     repl = gen_rtx_PLUS (Pmode, gen_rtx_REG (Pmode, STACK_POINTER_REGNUM),
> 			 GEN_INT (val));
> 
> which is better than
>   if (REGNO (reg) == STACK_POINTER_REGNUM)
>     repl = NULL_RTX;
>   else
>     repl = gen_rtx_PLUS (Pmode, gen_rtx_REG (Pmode, STACK_POINTER_REGNUM),
> 			 GEN_INT (val));

I think we're supposed to use a ? : thing for that.  Sucks as well; I would
try to rewrite the whole code to avoid such nasties.  Or move on :-)


Segher
diff mbox

Patch

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index fb522fb..67bcbc8 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -24712,7 +24712,7 @@  output_probe_stack_range (rtx reg1, rtx reg2)
 }
 
 /* Add to 'insn' a note which is PATTERN (INSN) but with REG replaced
-   with (plus:P (reg 1) VAL), and with REG2 replaced with RREG if REG2
+   with (plus:P (reg 1) VAL), and with REG2 replaced with REPL2 if REG2
    is not NULL.  It would be nice if dwarf2out_frame_debug_expr could
    deduce these equivalences by itself so it wasn't necessary to hold
    its hand so much.  Don't be tempted to always supply d2_f_d_e with
@@ -24722,22 +24722,28 @@  output_probe_stack_range (rtx reg1, rtx reg2)
 
 static rtx
 rs6000_frame_related (rtx insn, rtx reg, HOST_WIDE_INT val,
-		      rtx reg2, rtx rreg)
+		      rtx reg2, rtx repl2)
 {
-  rtx real, temp;
+  rtx patt, repl;
 
-  if (REGNO (reg) == STACK_POINTER_REGNUM && reg2 == NULL_RTX)
+  repl = NULL_RTX;
+  if (REGNO (reg) == STACK_POINTER_REGNUM)
+    gcc_checking_assert (val == 0);
+  else
+    repl = gen_rtx_PLUS (Pmode, gen_rtx_REG (Pmode, STACK_POINTER_REGNUM),
+			 GEN_INT (val));
+
+  patt = PATTERN (insn);
+  if (!repl && !reg2)
     {
       /* No need for any replacement.  Just set RTX_FRAME_RELATED_P.  */
       int i;
 
-      gcc_checking_assert (val == 0);
-      real = PATTERN (insn);
-      if (GET_CODE (real) == PARALLEL)
-	for (i = 0; i < XVECLEN (real, 0); i++)
-	  if (GET_CODE (XVECEXP (real, 0, i)) == SET)
+      if (GET_CODE (patt) == PARALLEL)
+	for (i = 0; i < XVECLEN (patt, 0); i++)
+	  if (GET_CODE (XVECEXP (patt, 0, i)) == SET)
 	    {
-	      rtx set = XVECEXP (real, 0, i);
+	      rtx set = XVECEXP (patt, 0, i);
 
 	      /* If this PARALLEL has been emitted for out-of-line
 		 register save functions, or store multiple, then omit
@@ -24752,79 +24758,49 @@  rs6000_frame_related (rtx insn, rtx reg, HOST_WIDE_INT val,
       return insn;
     }
 
-  /* copy_rtx will not make unique copies of registers, so we need to
-     ensure we don't have unwanted sharing here.  */
-  if (reg == reg2)
-    reg = gen_raw_REG (GET_MODE (reg), REGNO (reg));
-
-  if (reg == rreg)
-    reg = gen_raw_REG (GET_MODE (reg), REGNO (reg));
-
-  real = copy_rtx (PATTERN (insn));
-
-  if (reg2 != NULL_RTX)
-    real = replace_rtx (real, reg2, rreg);
-
-  if (REGNO (reg) == STACK_POINTER_REGNUM)
-    gcc_checking_assert (val == 0);
-  else
-    real = replace_rtx (real, reg,
-			gen_rtx_PLUS (Pmode, gen_rtx_REG (Pmode,
-							  STACK_POINTER_REGNUM),
-				      GEN_INT (val)));
-
-  /* We expect that 'real' is either a SET or a PARALLEL containing
+  /* We expect that 'patt' is either a SET or a PARALLEL containing
      SETs (and possibly other stuff).  In a PARALLEL, all the SETs
-     are important so they all have to be marked RTX_FRAME_RELATED_P.  */
+     are important so they all have to be marked RTX_FRAME_RELATED_P.
+     Call simplify_replace_rtx on the SETs rather than the whole insn
+     so as to leave the other stuff alone (for example USE of r12).  */
 
-  if (GET_CODE (real) == SET)
+  if (GET_CODE (patt) == SET)
     {
-      rtx set = real;
-
-      temp = simplify_rtx (SET_SRC (set));
-      if (temp)
-	SET_SRC (set) = temp;
-      temp = simplify_rtx (SET_DEST (set));
-      if (temp)
-	SET_DEST (set) = temp;
-      if (GET_CODE (SET_DEST (set)) == MEM)
-	{
-	  temp = simplify_rtx (XEXP (SET_DEST (set), 0));
-	  if (temp)
-	    XEXP (SET_DEST (set), 0) = temp;
-	}
+      if (repl)
+	patt = simplify_replace_rtx (patt, reg, repl);
+      if (reg2)
+	patt = simplify_replace_rtx (patt, reg2, repl2);
     }
-  else
+  else if (GET_CODE (patt) == PARALLEL)
     {
       int i;
 
-      gcc_assert (GET_CODE (real) == PARALLEL);
-      for (i = 0; i < XVECLEN (real, 0); i++)
-	if (GET_CODE (XVECEXP (real, 0, i)) == SET)
+      patt = shallow_copy_rtx (patt);
+      XVEC (patt, 0) = shallow_copy_rtvec (XVEC (patt, 0));
+
+      for (i = 0; i < XVECLEN (patt, 0); i++)
+	if (GET_CODE (XVECEXP (patt, 0, i)) == SET)
 	  {
-	    rtx set = XVECEXP (real, 0, i);
-
-	    temp = simplify_rtx (SET_SRC (set));
-	    if (temp)
-	      SET_SRC (set) = temp;
-	    temp = simplify_rtx (SET_DEST (set));
-	    if (temp)
-	      SET_DEST (set) = temp;
-	    if (GET_CODE (SET_DEST (set)) == MEM)
-	      {
-		temp = simplify_rtx (XEXP (SET_DEST (set), 0));
-		if (temp)
-		  XEXP (SET_DEST (set), 0) = temp;
-	      }
+	    rtx set = XVECEXP (patt, 0, i);
+
+	    if (repl)
+	      set = simplify_replace_rtx (set, reg, repl);
+	    if (reg2)
+	      set = simplify_replace_rtx (set, reg2, repl2);
+	    XVECEXP (patt, 0, i) = set;
+
 	    /* Omit eh_frame info for any user-defined global regs.  */
 	    if (!REG_P (SET_SRC (set))
 		|| !fixed_reg_p (REGNO (SET_SRC (set))))
 	      RTX_FRAME_RELATED_P (set) = 1;
 	  }
     }
+  else
+    gcc_unreachable ();
 
   RTX_FRAME_RELATED_P (insn) = 1;
-  add_reg_note (insn, REG_FRAME_RELATED_EXPR, real);
+  if (repl || reg2)
+    add_reg_note (insn, REG_FRAME_RELATED_EXPR, patt);
 
   return insn;
 }