diff mbox

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

Message ID 20150910005755.GA4330@bubble.grove.modra.org
State New
Headers show

Commit Message

Alan Modra Sept. 10, 2015, 12:57 a.m. UTC 
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.

Comments

David Edelsohn Sept. 10, 2015, 1:15 a.m. UTC | #1 
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
diff mbox

Patch

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