RTL cprop vs. fixed hard regs

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.

On Fri, Jan 30, 2015 at 01:05:54AM +1030, Alan Modra wrote:
> 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.

I'm still not happy about not constant propagating any fixed reg.
But if what you really care about is register variables (as in the
testcase), you could test for that?  Just global register vars or all
register vars, either works for me (not that I have to approve it ;-) )

Minor things:

> 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)])

HARD_REGISTER_P (x)

> 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* } { "*" } { "" } } */

You can drop the default args:
/* { dg-skip-if "" { powerpc*-*-darwin* } } */

Should you exclude darwin here, anyway?

> +/* { dg-options "-O2" } */
> +/* { dg-final { scan-assembler "ld 2,(24|40)\\(1\\)" } } */

To avoid "toothpick syndrome" you can group with {} instead of "".

Cheers,


Segher

On Thu, Jan 29, 2015 at 01:27:24PM -0600, Segher Boessenkool wrote:
> On Fri, Jan 30, 2015 at 01:05:54AM +1030, Alan Modra wrote:
> > 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.
> 
> I'm still not happy about not constant propagating any fixed reg.

s/constant/copy/, sorry.


Segher