[RS6000] Fix PowerPC ICE due to secondary_reload ignoring reload replacements

[RS6000] Fix PowerPC ICE due to secondary_reload ignoring reload replacements

[Alan Modra]

In https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67378 analysis I show
the reason for this PR is that insns emitted by secondary reload
patterns are being generated without taking into account other reloads
that may have occurred.  We run into this problem when an insn has a
pseudo that doesn't get a hard reg, and the pseudo is used in a way
that requires a secondary reload.  In this case the secondary reload
is needed due to gcc generating a 64-bit gpr load from memory insn
with an address offset not a multiple of 4.

Bootstrapped and regression tested powerpc64-linux.  OK to apply?
gcc-5 and gcc-4.9 branches too?

I haven't included a testcase in this patch, because the testcase in
the PR is quite horrible, and testcases triggering reload misbehaviour
tend to be unreliable.  By unreliable, I mean a small change anywhere
in the compiler can result in the testcase passing even if this bug
was reintroduced at some future date.  The testcase doesn't fail on
gcc-5, even though I'm fairly sure the same bug lurks there..

	PR target/67378
	* config/rs6000/rs6000.c (rs6000_secondary_reload_gpr): Find
	reload replacement for PRE_MODIFY address reg.

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index cfd5675..51046d4 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -18199,8 +18199,21 @@ rs6000_secondary_reload_gpr (rtx reg, rtx mem, rtx scratch, bool store_p)
 
   if (GET_CODE (addr) == PRE_MODIFY)
     {
+      gcc_assert (REG_P (XEXP (addr, 0))
+		  && GET_CODE (XEXP (addr, 1)) == PLUS
+		  && XEXP (XEXP (addr, 1), 0) == XEXP (addr, 0));
       scratch_or_premodify = XEXP (addr, 0);
-      gcc_assert (REG_P (scratch_or_premodify));
+      if (!HARD_REGISTER_P (scratch_or_premodify))
+	/* If we have a pseudo here then reload will have arranged
+	   to have it replaced, but only in the original insn.
+	   Use the replacement here too.  */
+	scratch_or_premodify = find_replacement (&XEXP (addr, 0));
+
+      /* RTL emitted by rs6000_secondary_reload_gpr uses RTL
+	 expressions from the original insn, without unsharing them.
+	 Any RTL that points into the original insn will of course
+	 have register replacements applied.  That is why we don't
+	 need to look for replacements under the PLUS.  */
       addr = XEXP (addr, 1);
     }
   gcc_assert (GET_CODE (addr) == PLUS || GET_CODE (addr) == LO_SUM);

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RS6000] Fix PowerPC ICE due to secondary_reload ignoring reload replacements

[David Edelsohn]

On Mon, Sep 7, 2015 at 11:47 PM, Alan Modra <amodra@gmail.com> wrote:
> In https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67378 analysis I show
> the reason for this PR is that insns emitted by secondary reload
> patterns are being generated without taking into account other reloads
> that may have occurred.  We run into this problem when an insn has a
> pseudo that doesn't get a hard reg, and the pseudo is used in a way
> that requires a secondary reload.  In this case the secondary reload
> is needed due to gcc generating a 64-bit gpr load from memory insn
> with an address offset not a multiple of 4.
>
> Bootstrapped and regression tested powerpc64-linux.  OK to apply?
> gcc-5 and gcc-4.9 branches too?
>
> I haven't included a testcase in this patch, because the testcase in
> the PR is quite horrible, and testcases triggering reload misbehaviour
> tend to be unreliable.  By unreliable, I mean a small change anywhere
> in the compiler can result in the testcase passing even if this bug
> was reintroduced at some future date.  The testcase doesn't fail on
> gcc-5, even though I'm fairly sure the same bug lurks there..
>
>         PR target/67378
>         * config/rs6000/rs6000.c (rs6000_secondary_reload_gpr): Find
>         reload replacement for PRE_MODIFY address reg.

I'm okay with this patch, but I'd like Uli to double-check it when he
has a moment.

Thanks, David

Re: [RS6000] Fix PowerPC ICE due to secondary_reload ignoring reload replacements

[Ulrich Weigand]

David Edelsohn wrote:
> On Mon, Sep 7, 2015 at 11:47 PM, Alan Modra <amodra@gmail.com> wrote:
> > In https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67378 analysis I show
> > the reason for this PR is that insns emitted by secondary reload
> > patterns are being generated without taking into account other reloads
> > that may have occurred.  We run into this problem when an insn has a
> > pseudo that doesn't get a hard reg, and the pseudo is used in a way
> > that requires a secondary reload.  In this case the secondary reload
> > is needed due to gcc generating a 64-bit gpr load from memory insn
> > with an address offset not a multiple of 4.
> >
> > Bootstrapped and regression tested powerpc64-linux.  OK to apply?
> > gcc-5 and gcc-4.9 branches too?
> >
> > I haven't included a testcase in this patch, because the testcase in
> > the PR is quite horrible, and testcases triggering reload misbehaviour
> > tend to be unreliable.  By unreliable, I mean a small change anywhere
> > in the compiler can result in the testcase passing even if this bug
> > was reintroduced at some future date.  The testcase doesn't fail on
> > gcc-5, even though I'm fairly sure the same bug lurks there..
> >
> >         PR target/67378
> >         * config/rs6000/rs6000.c (rs6000_secondary_reload_gpr): Find
> >         reload replacement for PRE_MODIFY address reg.
> 
> I'm okay with this patch, but I'd like Uli to double-check it when he
> has a moment.

The patch looks OK to me.  We definitely need to check for replacements
in secondary reload in such cases.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RS6000] Fix PowerPC ICE due to secondary_reload ignoring reload replacements

[Alan Modra]

On Tue, Sep 08, 2015 at 02:46:58PM +0200, Ulrich Weigand wrote:
> David Edelsohn wrote:
> > On Mon, Sep 7, 2015 at 11:47 PM, Alan Modra <amodra@gmail.com> wrote:
> > > In https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67378 analysis I show
> > > the reason for this PR is that insns emitted by secondary reload
> > > patterns are being generated without taking into account other reloads
> > > that may have occurred.  We run into this problem when an insn has a
> > > pseudo that doesn't get a hard reg, and the pseudo is used in a way
> > > that requires a secondary reload.  In this case the secondary reload
> > > is needed due to gcc generating a 64-bit gpr load from memory insn
> > > with an address offset not a multiple of 4.
> > >
> > >         PR target/67378
> > >         * config/rs6000/rs6000.c (rs6000_secondary_reload_gpr): Find
> > >         reload replacement for PRE_MODIFY address reg.
> > 
> > I'm okay with this patch, but I'd like Uli to double-check it when he
> > has a moment.
> 
> The patch looks OK to me.  We definitely need to check for replacements
> in secondary reload in such cases.

Thanks for reviewing!  I think rs6000_secondary_reload_inner needs the
same.  (https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01114.html
removed a bunch of find_replacement calls that I'd added previously.)

This patch reinstates some of the calls, a little more elegantly than
in my original effort.  I've also corrected an obvious error with the
PRE_DEC address offset.  Bootstrapped and regression tested
powerpc64le-linux.  OK for mainline and gcc-5?

	* config/rs6000/rs6000.c (rs6000_secondary_reload_inner): Find
	reload replacement for PRE_INC/DEC, PRE_MODIFY and AND address regs.
	Negate offset for PRE_DEC.

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index b9f35cd..f616b21 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -18020,7 +18020,12 @@ rs6000_secondary_reload_inner (rtx reg, rtx mem, rtx scratch, bool store_p)
 
       if ((addr_mask & RELOAD_REG_PRE_INCDEC) == 0)
 	{
-	  emit_insn (gen_add2_insn (op_reg, GEN_INT (GET_MODE_SIZE (mode))));
+	  if (!HARD_REGISTER_P (op_reg))
+	    op_reg = find_replacement (&XEXP (addr, 0));
+	  int delta = GET_MODE_SIZE (mode);
+	  if (GET_CODE (addr) == PRE_DEC)
+	    delta = -delta;
+	  emit_insn (gen_add2_insn (op_reg, GEN_INT (delta)));
 	  new_addr = op_reg;
 	}
       break;
@@ -18035,6 +18040,8 @@ rs6000_secondary_reload_inner (rtx reg, rtx mem, rtx scratch, bool store_p)
 
       if ((addr_mask & RELOAD_REG_PRE_MODIFY) == 0)
 	{
+	  if (!HARD_REGISTER_P (op0))
+	    op0 = find_replacement (&XEXP (addr, 0));
 	  emit_insn (gen_rtx_SET (op0, op1));
 	  new_addr = reg;
 	}
@@ -18048,7 +18055,11 @@ rs6000_secondary_reload_inner (rtx reg, rtx mem, rtx scratch, bool store_p)
       if ((addr_mask & RELOAD_REG_AND_M16) == 0)
 	{
 	  if (REG_P (op0) || GET_CODE (op0) == SUBREG)
-	    op_reg = op0;
+	    {
+	      op_reg = op0;
+	      if (!HARD_REGISTER_P (op_reg))
+		op_reg = find_replacement (&XEXP (addr, 0));
+	    }
 
 	  else if (GET_CODE (op1) == PLUS)
 	    {

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RS6000] Fix PowerPC ICE due to secondary_reload ignoring reload replacements

[David Edelsohn]

On Wed, Sep 9, 2015 at 8:57 PM, Alan Modra <amodra@gmail.com> wrote:
> On Tue, Sep 08, 2015 at 02:46:58PM +0200, Ulrich Weigand wrote:
>> David Edelsohn wrote:
>> > On Mon, Sep 7, 2015 at 11:47 PM, Alan Modra <amodra@gmail.com> wrote:
>> > > In https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67378 analysis I show
>> > > the reason for this PR is that insns emitted by secondary reload
>> > > patterns are being generated without taking into account other reloads
>> > > that may have occurred.  We run into this problem when an insn has a
>> > > pseudo that doesn't get a hard reg, and the pseudo is used in a way
>> > > that requires a secondary reload.  In this case the secondary reload
>> > > is needed due to gcc generating a 64-bit gpr load from memory insn
>> > > with an address offset not a multiple of 4.
>> > >
>> > >         PR target/67378
>> > >         * config/rs6000/rs6000.c (rs6000_secondary_reload_gpr): Find
>> > >         reload replacement for PRE_MODIFY address reg.
>> >
>> > I'm okay with this patch, but I'd like Uli to double-check it when he
>> > has a moment.
>>
>> The patch looks OK to me.  We definitely need to check for replacements
>> in secondary reload in such cases.
>
> Thanks for reviewing!  I think rs6000_secondary_reload_inner needs the
> same.  (https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01114.html
> removed a bunch of find_replacement calls that I'd added previously.)
>
> This patch reinstates some of the calls, a little more elegantly than
> in my original effort.  I've also corrected an obvious error with the
> PRE_DEC address offset.  Bootstrapped and regression tested
> powerpc64le-linux.  OK for mainline and gcc-5?

Alan,

You and Mike need to get on the same page.  I don't want ping-ponging
patches where you add a check and Mike knowingly or unknowingly
removes it, then you add it back.

Ideally I want a testcase.  Barring that, I want a comment at all of
these points explaining why find_replacement is necessary.

Thanks, David