RTL cprop vs. fixed hard regs

[Alan Modra]

On Sat, Jan 17, 2015 at 01:12:49PM +0100, Richard Biener wrote:
> On January 17, 2015 1:37:12 AM CET, Alan Modra <amodra@gmail.com> wrote:
> >On Fri, Jan 16, 2015 at 11:03:24AM -0600, Segher Boessenkool wrote:
> >> On Fri, Jan 16, 2015 at 08:12:27PM +1030, Alan Modra wrote:
> >> > OK, so we need to fix this in the rs6000 backend, but it occurs to
> >me
> >> > that cprop also has a bug here.  It shouldn't be touching fixed
> >hard
> >> > registers.
> >> 
> >> Why not?  It cannot allocate a fixed reg to a pseudo, but other than
> >> that there is nothing special about fixed regs; the transform is
> >> perfectly valid as far as I see.
> >
> >I didn't say that copying to a pseudo and using that was invalid..
> >The bug I see is a mis-optimisation.  Also, the asm operands case that
> >do_local_cprop already rules out changing is very similar to fixed
> >regs.  Would you argue that changing asm operands is also valid?  :)
> >
> >> It isn't a desirable transform in this case, but that is not true for
> >> fixed regs in general (just because the stack pointer is live
> >everywhere).
> >
> >What's the point in extending the lifetime of some pseudo when you
> >know the original fixed register is available everywhere?  Do you have
> >some concrete example in mind where this "optimisation" is beneficial?
> >
> >Some ports even include pc in fixed_regs.  So there are obvious
> >examples where regs in fixed_regs change behind the compiler's back.
> >Naive users might even expect to see the "current" value of those
> >regs.  (Again, I'm not saying that it is invalid if gcc substituted an
> >older value.)
> 
> Just to add, we avoid doing this on the GIMPLE level as well.

Segher, does this one look better to you?  The previous patch turned
off constant propagation for fixed registers as well as register copy
propagation.  The latter is all I really meant to do.
Bootstrapped etc. powerpc64-linux.

gcc/
	* cprop.c (do_local_cprop): Disallow register copy propagation
	of fixed hard registers.
gcc/testsuite/
	* gcc.target/powerpc/cprophard.c: New.

Index: gcc/cprop.c
===================================================================
--- gcc/cprop.c	(revision 219792)
+++ gcc/cprop.c	(working copy)
@@ -1207,7 +1207,11 @@
 
 	  if (cprop_constant_p (this_rtx))
 	    newcnst = this_rtx;
-	  if (REG_P (this_rtx) && REGNO (this_rtx) >= FIRST_PSEUDO_REGISTER
+	  if (REG_P (this_rtx)
+	      && REGNO (this_rtx) >= FIRST_PSEUDO_REGISTER
+	      /* Don't copy propagate fixed regs.  This just tends to
+		 extend the lifetime of this_rtx to no purpose.  */
+	      && (REGNO (x) >= FIRST_PSEUDO_REGISTER || !fixed_regs[REGNO (x)])
 	      /* Don't copy propagate if it has attached REG_EQUIV note.
 		 At this point this only function parameters should have
 		 REG_EQUIV notes and if the argument slot is used somewhere
Index: gcc/testsuite/gcc.target/powerpc/cprophard.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/cprophard.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/cprophard.c	(working copy)
@@ -0,0 +1,13 @@
+/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */
+/* { dg-options "-O2" } */
+/* { dg-final { scan-assembler "ld 2,(24|40)\\(1\\)" } } */
+
+/* From a linux kernel mis-compile of net/core/skbuff.c.  */
+register unsigned long current_r1 asm ("r1");
+
+void f (unsigned int n, void (*fun) (unsigned long))
+{
+  while (n--)
+    (*fun) (current_r1 & -0x1000);
+}


-- 
Alan Modra
Australia Development Lab, IBM