RTL cprop vs. fixed hard regs

RTL cprop vs. fixed hard regs

[Alan Modra]

https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-December/123776.html
shows gcc-5 miscompiling a powerpc64 linux kernel.  The executive
summary is that the rs6000 backend has a bug in its RTL description of
indirect calls.  We specify a parallel containing both the actual call
and an action that happens after the call, the restore of r2.  The
restore is simply a memory load:
            (set (reg:DI 2 2)
                (mem/v/c:DI (plus:DI (reg/f:DI 1 1)
                        (const_int 40 [0x28])) [0  S8 A8]))
This leads to cprop concluding that it is valid to replace the
reference to r1 with another register having the same value before the
call.  Unfortunately, sometimes a call-clobbered register is chosen.

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.  Bootstrapped and regression tested powerpc64-linux.  OK
for mainline?

	* cprop.c (do_local_cprop): Disallow replacement of fixed
	hard registers.

Index: gcc/cprop.c
===================================================================
--- gcc/cprop.c	(revision 219662)
+++ gcc/cprop.c	(working copy)
@@ -1189,10 +1189,12 @@ do_local_cprop (rtx x, rtx_insn *insn)
   rtx newreg = NULL, newcnst = NULL;
 
   /* Rule out USE instructions and ASM statements as we don't want to
-     change the hard registers mentioned.  */
+     change the hard registers mentioned, and don't change fixed hard
+     registers.  */
   if (REG_P (x)
       && (REGNO (x) >= FIRST_PSEUDO_REGISTER
           || (GET_CODE (PATTERN (insn)) != USE
+	      && !fixed_regs[REGNO (x)]
 	      && asm_noperands (PATTERN (insn)) < 0)))
     {
       cselib_val *val = cselib_lookup (x, GET_MODE (x), 0, VOIDmode);

-- 
Alan Modra
Australia Development Lab, IBM

Re: RTL cprop vs. fixed hard regs

[Jeff Law]

On 01/16/15 02:42, Alan Modra wrote:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-December/123776.html
> shows gcc-5 miscompiling a powerpc64 linux kernel.  The executive
> summary is that the rs6000 backend has a bug in its RTL description of
> indirect calls.  We specify a parallel containing both the actual call
> and an action that happens after the call, the restore of r2.  The
> restore is simply a memory load:
>              (set (reg:DI 2 2)
>                  (mem/v/c:DI (plus:DI (reg/f:DI 1 1)
>                          (const_int 40 [0x28])) [0  S8 A8]))
> This leads to cprop concluding that it is valid to replace the
> reference to r1 with another register having the same value before the
> call.  Unfortunately, sometimes a call-clobbered register is chosen.
>
> 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.  Bootstrapped and regression tested powerpc64-linux.  OK
> for mainline?
>
> 	* cprop.c (do_local_cprop): Disallow replacement of fixed
> 	hard registers.
OK.  Extra credit for a testcase, ppc specific is obviously OK.

jeff

Re: RTL cprop vs. fixed hard regs

[Segher Boessenkool]

On Fri, Jan 16, 2015 at 08:12:27PM +1030, Alan Modra wrote:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-December/123776.html
> shows gcc-5 miscompiling a powerpc64 linux kernel.  The executive
> summary is that the rs6000 backend has a bug in its RTL description of
> indirect calls.  We specify a parallel containing both the actual call
> and an action that happens after the call, the restore of r2.  The
> restore is simply a memory load:
>             (set (reg:DI 2 2)
>                 (mem/v/c:DI (plus:DI (reg/f:DI 1 1)
>                         (const_int 40 [0x28])) [0  S8 A8]))
> This leads to cprop concluding that it is valid to replace the
> reference to r1 with another register having the same value before the
> call.  Unfortunately, sometimes a call-clobbered register is chosen.
> 
> 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.

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


Segher

Re: RTL cprop vs. fixed hard regs

[Alan Modra]

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

-- 
Alan Modra
Australia Development Lab, IBM

Re: RTL cprop vs. fixed hard regs

[Alan Modra]

On Fri, Jan 16, 2015 at 09:35:16AM -0700, Jeff Law wrote:
> On 01/16/15 02:42, Alan Modra wrote:
> >	* cprop.c (do_local_cprop): Disallow replacement of fixed
> >	hard registers.
> OK.  Extra credit for a testcase, ppc specific is obviously OK.

Thanks.  Committed revision 219786.  I'll see if I can come up with a
reasonable testcase.

-- 
Alan Modra
Australia Development Lab, IBM

Re: RTL cprop vs. fixed hard regs

[Segher Boessenkool]

On Sat, Jan 17, 2015 at 11:07:12AM +1030, Alan Modra 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.

Ah, okay, good :-)

This same mis-optimisation would happen if r1 was just some regular
non-fixed register, hrm.  Maybe something else in cprop needs some
tuning up?

> 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?  :)

A fixed reg in an asm_operands is a hard reg; a hard reg in an asm_operands
(before reload) is a register asm variable.  And we had better not change
register variable asm arguments, since that is what we promise not to do
with register variables.  The case is not similar at all.

> > 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?

That is my point: _if_ you know it is live all the time, or if there is no
advantage to shortening the lifetime of the value in that fixed reg, then
yes we should not propagate that value.  But that is not true for all
fixed regs.

> Do you have
> some concrete example in mind where this "optimisation" is beneficial?

The CA_REG in rs6000 is a fixed register.  It isn't a terribly good
example because it cannot be propagated anyway, for other reasons; but
it will hopefully help explain my point.  So please pretend we can copy
it to GPRs :-)  [ The situation with the T bit on SH is similar, but I
don't know the details there well enough. ]

There is only one CA_REG.  It is used in quite a few sequences.  It
contains a totally different value every time.  Because there is only
one such register the instruction sequences around it cannot be reordered
very well.

Propagating the value in such a not-so-very-fixed fixed reg helps reduce
the lifetimes of the values in those regs, helps reordering, combining,
scheduling, performance in general.

If you are only concerned about the stack pointer, you could just check
for that?  But please add a comment in any case, saying why you exclude
it (and ideally don't lump it in with tests that are needed for
correctness).

Cheers,


Segher

Re: RTL cprop vs. fixed hard regs

[Alan Modra]

On Fri, Jan 16, 2015 at 08:09:51PM -0600, Segher Boessenkool wrote:
> On Sat, Jan 17, 2015 at 11:07:12AM +1030, Alan Modra 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.
> 
> Ah, okay, good :-)
> 
> This same mis-optimisation would happen if r1 was just some regular
> non-fixed register, hrm.  Maybe something else in cprop needs some
> tuning up?

Well, if the original pseudo register dies earlier as a result of
substituting a copy then you've gained.

> > 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?  :)
> 
> A fixed reg in an asm_operands is a hard reg; a hard reg in an asm_operands
> (before reload) is a register asm variable.  And we had better not change
> register variable asm arguments, since that is what we promise not to do
> with register variables.  The case is not similar at all.

The similarity I see is that we have a hard reg that is a register asm
variable here too.  How else do you get a copy from a fixed hard reg
to a pseudo?  Hrrm, maybe some backend code does that sort of thing.

> > > 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?
> 
> That is my point: _if_ you know it is live all the time, or if there is no
> advantage to shortening the lifetime of the value in that fixed reg, then
> yes we should not propagate that value.  But that is not true for all
> fixed regs.
> 
> > Do you have
> > some concrete example in mind where this "optimisation" is beneficial?
> 
> The CA_REG in rs6000 is a fixed register.  It isn't a terribly good
> example because it cannot be propagated anyway, for other reasons; but
> it will hopefully help explain my point.  So please pretend we can copy
> it to GPRs :-)  [ The situation with the T bit on SH is similar, but I
> don't know the details there well enough. ]
> 
> There is only one CA_REG.  It is used in quite a few sequences.  It
> contains a totally different value every time.  Because there is only
> one such register the instruction sequences around it cannot be reordered
> very well.
> 
> Propagating the value in such a not-so-very-fixed fixed reg helps reduce
> the lifetimes of the values in those regs, helps reordering, combining,
> scheduling, performance in general.
> 
> If you are only concerned about the stack pointer, you could just check
> for that?  But please add a comment in any case, saying why you exclude
> it (and ideally don't lump it in with tests that are needed for
> correctness).

No, I don't want to special-case sp.  That's horrible.  If the patch I
just committed is wrong, I'll revert it.

-- 
Alan Modra
Australia Development Lab, IBM

Re: RTL cprop vs. fixed hard regs

[Alan Modra]

On Sat, Jan 17, 2015 at 11:16:57AM +1030, Alan Modra wrote:
> On Fri, Jan 16, 2015 at 09:35:16AM -0700, Jeff Law wrote:
> > On 01/16/15 02:42, Alan Modra wrote:
> > >	* cprop.c (do_local_cprop): Disallow replacement of fixed
> > >	hard registers.
> > OK.  Extra credit for a testcase, ppc specific is obviously OK.
> 
> Thanks.  Committed revision 219786.  I'll see if I can come up with a
> reasonable testcase.

And now reverted due to Segher's objection.  Here's the testcase FWIW.

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 { powerpc64*-*-* && 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

Re: RTL cprop vs. fixed hard regs

[Richard Biener]

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.

Richard.

Re: 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

Re: RTL cprop vs. fixed hard regs

[Segher Boessenkool]

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

Re: RTL cprop vs. fixed hard regs

[Segher Boessenkool]

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

[RS6000] fix for RTL cprop vs. fixed hard regs

[Alan Modra]

On Fri, Jan 16, 2015 at 08:12:27PM +1030, Alan Modra wrote:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-December/123776.html
> shows gcc-5 miscompiling a powerpc64 linux kernel.  The executive
> summary is that the rs6000 backend has a bug in its RTL description of
> indirect calls.  We specify a parallel containing both the actual call
> and an action that happens after the call, the restore of r2.  The
> restore is simply a memory load:
>             (set (reg:DI 2 2)
>                 (mem/v/c:DI (plus:DI (reg/f:DI 1 1)
>                         (const_int 40 [0x28])) [0  S8 A8]))
> This leads to cprop concluding that it is valid to replace the
> reference to r1 with another register having the same value before the
> call.  Unfortunately, sometimes a call-clobbered register is chosen.

This is the rs6000 backend fix.  Bootstrapped etc. powerpc64-linux.
OK to apply?

gcc/
	* config/rs6000/rs6000.c (rs6000_call_aix): Use unspec rather
	than mem for toc_restore.
	* config/rs6000/rs6000.md (UNSPEC_TOCSLOT): Define.
	(call_indirect_aix, call_value_indirect_aix): Adjust to suit.
	(call_indirect_elfv2, call_value_indirect_elfv2): Likewise.
gcc/testsuite/
	* gcc.target/powerpc/cprophard.c: New.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 220025)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -32934,7 +32934,10 @@ rs6000_call_aix (rtx value, rtx func_desc, rtx fla
       rtx stack_toc_mem = gen_frame_mem (Pmode,
 					 gen_rtx_PLUS (Pmode, stack_ptr,
 						       stack_toc_offset));
-      toc_restore = gen_rtx_SET (VOIDmode, toc_reg, stack_toc_mem);
+      rtx stack_toc_unspec = gen_rtx_UNSPEC (Pmode,
+					     gen_rtvec (1, stack_toc_offset),
+					     UNSPEC_TOCSLOT);
+      toc_restore = gen_rtx_SET (VOIDmode, toc_reg, stack_toc_unspec);
 
       /* Can we optimize saving the TOC in the prologue or
 	 do we need to do it at every call?  */
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 220025)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -138,6 +138,7 @@
    UNSPEC_PACK_128BIT
    UNSPEC_LSQ
    UNSPEC_FUSION_GPR
+   UNSPEC_TOCSLOT
   ])
 
 ;;
@@ -11348,16 +11349,16 @@
 ;; Call to indirect functions with the AIX abi using a 3 word descriptor.
 ;; Operand0 is the addresss of the function to call
 ;; Operand2 is the location in the function descriptor to load r2 from
-;; Operand3 is the stack location to hold the current TOC pointer
+;; Operand3 is the offset of the stack location holding the current TOC pointer
 
 (define_insn "*call_indirect_aix<mode>"
   [(call (mem:SI (match_operand:P 0 "register_operand" "c,*l"))
 	 (match_operand 1 "" "g,g"))
    (use (match_operand:P 2 "memory_operand" "<ptrm>,<ptrm>"))
-   (set (reg:P TOC_REGNUM) (match_operand:P 3 "memory_operand" "<ptrm>,<ptrm>"))
+   (set (reg:P TOC_REGNUM) (unspec [(match_operand:P 3 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
    (clobber (reg:P LR_REGNO))]
   "DEFAULT_ABI == ABI_AIX"
-  "<ptrload> 2,%2\;b%T0l\;<ptrload> 2,%3"
+  "<ptrload> 2,%2\;b%T0l\;<ptrload> 2,%3(1)"
   [(set_attr "type" "jmpreg")
    (set_attr "length" "12")])
 
@@ -11366,24 +11367,24 @@
 	(call (mem:SI (match_operand:P 1 "register_operand" "c,*l"))
 	      (match_operand 2 "" "g,g")))
    (use (match_operand:P 3 "memory_operand" "<ptrm>,<ptrm>"))
-   (set (reg:P TOC_REGNUM) (match_operand:P 4 "memory_operand" "<ptrm>,<ptrm>"))
+   (set (reg:P TOC_REGNUM) (unspec [(match_operand:P 4 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
    (clobber (reg:P LR_REGNO))]
   "DEFAULT_ABI == ABI_AIX"
-  "<ptrload> 2,%3\;b%T1l\;<ptrload> 2,%4"
+  "<ptrload> 2,%3\;b%T1l\;<ptrload> 2,%4(1)"
   [(set_attr "type" "jmpreg")
    (set_attr "length" "12")])
 
 ;; Call to indirect functions with the ELFv2 ABI.
 ;; Operand0 is the addresss of the function to call
-;; Operand2 is the stack location to hold the current TOC pointer
+;; Operand2 is the offset of the stack location holding the current TOC pointer
 
 (define_insn "*call_indirect_elfv2<mode>"
   [(call (mem:SI (match_operand:P 0 "register_operand" "c,*l"))
 	 (match_operand 1 "" "g,g"))
-   (set (reg:P TOC_REGNUM) (match_operand:P 2 "memory_operand" "<ptrm>,<ptrm>"))
+   (set (reg:P TOC_REGNUM) (unspec [(match_operand:P 2 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
    (clobber (reg:P LR_REGNO))]
   "DEFAULT_ABI == ABI_ELFv2"
-  "b%T0l\;<ptrload> 2,%2"
+  "b%T0l\;<ptrload> 2,%2(1)"
   [(set_attr "type" "jmpreg")
    (set_attr "length" "8")])
 
@@ -11391,10 +11392,10 @@
   [(set (match_operand 0 "" "")
 	(call (mem:SI (match_operand:P 1 "register_operand" "c,*l"))
 	      (match_operand 2 "" "g,g")))
-   (set (reg:P TOC_REGNUM) (match_operand:P 3 "memory_operand" "<ptrm>,<ptrm>"))
+   (set (reg:P TOC_REGNUM) (unspec [(match_operand:P 3 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
    (clobber (reg:P LR_REGNO))]
   "DEFAULT_ABI == ABI_ELFv2"
-  "b%T1l\;<ptrload> 2,%3"
+  "b%T1l\;<ptrload> 2,%3(1)"
   [(set_attr "type" "jmpreg")
    (set_attr "length" "8")])
 
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

Re: [RS6000] fix for RTL cprop vs. fixed hard regs

[David Edelsohn]

On Fri, Jan 30, 2015 at 1:57 AM, Alan Modra <amodra@gmail.com> wrote:
> On Fri, Jan 16, 2015 at 08:12:27PM +1030, Alan Modra wrote:
>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-December/123776.html
>> shows gcc-5 miscompiling a powerpc64 linux kernel.  The executive
>> summary is that the rs6000 backend has a bug in its RTL description of
>> indirect calls.  We specify a parallel containing both the actual call
>> and an action that happens after the call, the restore of r2.  The
>> restore is simply a memory load:
>>             (set (reg:DI 2 2)
>>                 (mem/v/c:DI (plus:DI (reg/f:DI 1 1)
>>                         (const_int 40 [0x28])) [0  S8 A8]))
>> This leads to cprop concluding that it is valid to replace the
>> reference to r1 with another register having the same value before the
>> call.  Unfortunately, sometimes a call-clobbered register is chosen.
>
> This is the rs6000 backend fix.  Bootstrapped etc. powerpc64-linux.
> OK to apply?
>
> gcc/
>         * config/rs6000/rs6000.c (rs6000_call_aix): Use unspec rather
>         than mem for toc_restore.
>         * config/rs6000/rs6000.md (UNSPEC_TOCSLOT): Define.

Please insert UNSPEC_TOCSLOT in the UNSPEC list after UNSPEC_TOCPTR
and UNSPEC_TOC and add  a short comment like the other two.

>         (call_indirect_aix, call_value_indirect_aix): Adjust to suit.
>         (call_indirect_elfv2, call_value_indirect_elfv2): Likewise.
> gcc/testsuite/
>         * gcc.target/powerpc/cprophard.c: New.

Okay with that change.

Thanks, David