[RS6000] power8 internal compiler errors

[RS6000] power8 internal compiler errors

[Alan Modra]

Since reload may make multiple passes through insns, rtl seen during
reload can look a little messy.  On the second and subsequent passes
you get to see any transformations made on previous passes.  The rtl
sanity checks in rs6000_secondary_reload_inner didn't take this fact
into account, leading to these PRs..

So, sanity check the rtl as it will be after reload.  It is also
correct for any insn emitted in rs6000_secondary_reload_inner to use
the final rtl too.  Bootstrapped and regression tested
powerpc64-linux.  OK to apply?

	PR target/58675
	PR target/57935
	* config/rs6000/rs6000.c (rs6000_secondary_reload_inner): Use
	find_replacement on parts of insn rtl that might be reloaded.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 207649)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -16170,7 +16170,7 @@
     rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
 
   rclass = REGNO_REG_CLASS (regno);
-  addr = XEXP (mem, 0);
+  addr = find_replacement (&XEXP (mem, 0));
 
   switch (rclass)
     {
@@ -16181,7 +16181,7 @@
       if (GET_CODE (addr) == AND)
 	{
 	  and_op2 = XEXP (addr, 1);
-	  addr = XEXP (addr, 0);
+	  addr = find_replacement (&XEXP (addr, 0));
 	}
 
       if (GET_CODE (addr) == PRE_MODIFY)
@@ -16190,10 +16190,9 @@
 	  if (!REG_P (scratch_or_premodify))
 	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
 
-	  if (GET_CODE (XEXP (addr, 1)) != PLUS)
-	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
-
 	  addr = XEXP (addr, 1);
+	  if (GET_CODE (addr) != PLUS)
+	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
 	}
 
       if (GET_CODE (addr) == PLUS
@@ -16201,7 +16200,7 @@
 	      || !rs6000_legitimate_offset_address_p (PTImode, addr,
 						      false, true)))
 	{
-	  addr_op1 = XEXP (addr, 0);
+	  addr_op1 = find_replacement (&XEXP (addr, 0));
 	  addr_op2 = XEXP (addr, 1);
 	  if (!legitimate_indirect_address_p (addr_op1, false))
 	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
@@ -16276,7 +16275,7 @@
 	      || !VECTOR_MEM_ALTIVEC_P (mode)))
 	{
 	  and_op2 = XEXP (addr, 1);
-	  addr = XEXP (addr, 0);
+	  addr = find_replacement (&XEXP (addr, 0));
 	}
 
       /* If we aren't using a VSX load, save the PRE_MODIFY register and use it
@@ -16292,10 +16291,9 @@
 	  if (!legitimate_indirect_address_p (scratch_or_premodify, false))
 	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
 
-	  if (GET_CODE (XEXP (addr, 1)) != PLUS)
-	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
-
 	  addr = XEXP (addr, 1);
+	  if (GET_CODE (addr) != PLUS)
+	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
 	}
 
       if (legitimate_indirect_address_p (addr, false)	/* reg */
@@ -16310,7 +16308,7 @@
 
       else if (GET_CODE (addr) == PLUS)
 	{
-	  addr_op1 = XEXP (addr, 0);
+	  addr_op1 = find_replacement (&XEXP (addr, 0));
 	  addr_op2 = XEXP (addr, 1);
 	  if (!REG_P (addr_op1))
 	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RS6000] power8 internal compiler errors

[David Edelsohn]

On Mon, Feb 10, 2014 at 5:18 PM, Alan Modra <amodra@gmail.com> wrote:

Shouldn't addr_op2 also be set from find_replacement?

- David

> @@ -16201,7 +16200,7 @@
>               || !rs6000_legitimate_offset_address_p (PTImode, addr,
>                                                       false, true)))
>         {
> -         addr_op1 = XEXP (addr, 0);
> +         addr_op1 = find_replacement (&XEXP (addr, 0));
>           addr_op2 = XEXP (addr, 1);
^^^^^
>           if (!legitimate_indirect_address_p (addr_op1, false))
>             rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);

> @@ -16310,7 +16308,7 @@
>
>        else if (GET_CODE (addr) == PLUS)
>         {
> -         addr_op1 = XEXP (addr, 0);
> +         addr_op1 = find_replacement (&XEXP (addr, 0));
>           addr_op2 = XEXP (addr, 1);
^^^^^
>           if (!REG_P (addr_op1))
>             rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);

Re: [RS6000] power8 internal compiler errors

[Alan Modra]

On Mon, Feb 10, 2014 at 07:01:03PM -0500, David Edelsohn wrote:
> On Mon, Feb 10, 2014 at 5:18 PM, Alan Modra <amodra@gmail.com> wrote:
> 
> Shouldn't addr_op2 also be set from find_replacement?

Sorry, I thought after I sent the email that I should have added some
explanation of why certain parts need find_replacement and others
don't.  We want just those parts of addresses that might have been
reloaded.

There's the case of the entire address being reloaded (actually, I'm
not sure this one is needed) and then all the ones we do in the rs6000
backend in legitimize_reload_address.  I think I found all the
required parts but it certainly won't hurt if you check too.  Calling
find_replacement when not strictly necessary will slow down gcc a
little..

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RS6000] power8 internal compiler errors

[David Edelsohn]

On Mon, Feb 10, 2014 at 8:33 PM, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Feb 10, 2014 at 07:01:03PM -0500, David Edelsohn wrote:
>> On Mon, Feb 10, 2014 at 5:18 PM, Alan Modra <amodra@gmail.com> wrote:
>>
>> Shouldn't addr_op2 also be set from find_replacement?
>
> Sorry, I thought after I sent the email that I should have added some
> explanation of why certain parts need find_replacement and others
> don't.  We want just those parts of addresses that might have been
> reloaded.
>
> There's the case of the entire address being reloaded (actually, I'm
> not sure this one is needed) and then all the ones we do in the rs6000
> backend in legitimize_reload_address.  I think I found all the
> required parts but it certainly won't hurt if you check too.  Calling
> find_replacement when not strictly necessary will slow down gcc a
> little..

It looked like either piece of the address could be a more complicated
address that is reloaded, especially in a failure case, at least based
on the few other uses of find_replacement in other backends. But it is
difficult to determine.

The secondary_reload_inner code seems to be prepared for more
complicated addresses that could require reloads in the second
operand, but I agree that legitimize_reload_address should affect the
first operand.

- David

Re: [RS6000] power8 internal compiler errors

[David Edelsohn]

On Mon, Feb 10, 2014 at 8:33 PM, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Feb 10, 2014 at 07:01:03PM -0500, David Edelsohn wrote:
>> On Mon, Feb 10, 2014 at 5:18 PM, Alan Modra <amodra@gmail.com> wrote:
>>
>> Shouldn't addr_op2 also be set from find_replacement?
>
> Sorry, I thought after I sent the email that I should have added some
> explanation of why certain parts need find_replacement and others
> don't.  We want just those parts of addresses that might have been
> reloaded.
>
> There's the case of the entire address being reloaded (actually, I'm
> not sure this one is needed) and then all the ones we do in the rs6000
> backend in legitimize_reload_address.  I think I found all the
> required parts but it certainly won't hurt if you check too.  Calling
> find_replacement when not strictly necessary will slow down gcc a
> little..

I cannot tell if either branch of the PLUS could contain an address
that previously was replaced. I want to avoid slow downs, but I also
want to avoid leaving a latent bug, either now or a future change to
reload or IRA.

Is there an ENABLE_CHECKING assert of some form that could test
addr_op2 to ensure that it does contain part of the address that was
reloaded when not configured as a Release?

Thanks, David

Re: [RS6000] power8 internal compiler errors

[Ulrich Weigand]

David Edelsohn wrote:
> On Mon, Feb 10, 2014 at 8:33 PM, Alan Modra <amodra@gmail.com> wrote:
> > On Mon, Feb 10, 2014 at 07:01:03PM -0500, David Edelsohn wrote:
> >> On Mon, Feb 10, 2014 at 5:18 PM, Alan Modra <amodra@gmail.com> wrote:
> >>
> >> Shouldn't addr_op2 also be set from find_replacement?
> >
> > Sorry, I thought after I sent the email that I should have added some
> > explanation of why certain parts need find_replacement and others
> > don't.  We want just those parts of addresses that might have been
> > reloaded.
> >
> > There's the case of the entire address being reloaded (actually, I'm
> > not sure this one is needed) and then all the ones we do in the rs6000
> > backend in legitimize_reload_address.  I think I found all the
> > required parts but it certainly won't hurt if you check too.  Calling
> > find_replacement when not strictly necessary will slow down gcc a
> > little..
> 
> I cannot tell if either branch of the PLUS could contain an address
> that previously was replaced. I want to avoid slow downs, but I also
> want to avoid leaving a latent bug, either now or a future change to
> reload or IRA.

In general, any part of an address may have been replaced.  In the
most generic case, if we have an address of the form
  (mem (plus (reg1) (reg2)))
for two pseudos, each of the pseudos *could* have been allocated
e.g. to an FPR under unusual circumstances, and then it would
require a replacement (without the whole address itself being
fully replaced).
 
> Is there an ENABLE_CHECKING assert of some form that could test
> addr_op2 to ensure that it does contain part of the address that was
> reloaded when not configured as a Release?

Note that find_replacement itself already recurses into both sides
of a PLUS.  So given the code flow:

-  addr = XEXP (mem, 0);
+  addr = find_replacement (&XEXP (mem, 0));
[...]
       if (GET_CODE (addr) == AND)
 	{
 	  and_op2 = XEXP (addr, 1);
-	  addr = XEXP (addr, 0);
+	  addr = find_replacement (&XEXP (addr, 0));
 	}

      if (GET_CODE (addr) == PRE_MODIFY)
        {
          scratch_or_premodify = XEXP (addr, 0);
          if (!REG_P (scratch_or_premodify))
            rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);

          if (GET_CODE (XEXP (addr, 1)) != PLUS)
            rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);

          addr = XEXP (addr, 1);
        }

      if (GET_CODE (addr) == PLUS
          && (and_op2 != NULL_RTX
              || !rs6000_legitimate_offset_address_p (PTImode, addr,
                                                      false, true)))
        {
          addr_op1 = XEXP (addr, 0);
          addr_op2 = XEXP (addr, 1);

the only time addr_op1 *or* addr_op2 might need another replacement
is if addr was reset to the inner of a PRE_MODIFY.  So it might be
easier and cheaper overall to just do a find_replacement within
the PRE_MODIFY clause ...


If you really prefer a check, I guess you can always do something like:

#ifdef ENABLE_CHECKING
  gcc_assert (find_replacement (&XEXP (...)) == XEXP (...));
#endif

Bye,
Ulrich

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

Re: [RS6000] power8 internal compiler errors

[Alan Modra]

On Wed, Feb 12, 2014 at 06:47:37PM +0100, Ulrich Weigand wrote:
> Note that find_replacement itself already recurses into both sides
> of a PLUS.

Thanks, I missed seeing that.  I'd analysed the bug and knew what
needed doing from past forays into reload, so went looking for ways to
get at the reloads, ie. "replacements" at that stage of reload.  Lo
and behold, there's a function tailor made to do just that!  So I
plugged in find_replacements() wherever it seemed necessary.

> So it might be
> easier and cheaper overall to just do a find_replacement within
> the PRE_MODIFY clause ...

That's a good idea, since PRE_MODIFY doesn't occur that often.
Here is the revised patch with your recommendations.  Bootstrapped
and regression tested powerpc64-linux.

	PR target/58675
	PR target/57935
	* config/rs6000/rs6000.c (rs6000_secondary_reload_inner): Use
	find_replacement on parts of insn rtl that might be reloaded.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 207649)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -16170,7 +16156,7 @@ rs6000_secondary_reload_inner (rtx reg, rtx mem, r
     rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
 
   rclass = REGNO_REG_CLASS (regno);
-  addr = XEXP (mem, 0);
+  addr = find_replacement (&XEXP (mem, 0));
 
   switch (rclass)
     {
@@ -16181,19 +16167,18 @@ rs6000_secondary_reload_inner (rtx reg, rtx mem, r
       if (GET_CODE (addr) == AND)
 	{
 	  and_op2 = XEXP (addr, 1);
-	  addr = XEXP (addr, 0);
+	  addr = find_replacement (&XEXP (addr, 0));
 	}
 
       if (GET_CODE (addr) == PRE_MODIFY)
 	{
-	  scratch_or_premodify = XEXP (addr, 0);
+	  scratch_or_premodify = find_replacement (&XEXP (addr, 0));
 	  if (!REG_P (scratch_or_premodify))
 	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
 
-	  if (GET_CODE (XEXP (addr, 1)) != PLUS)
+	  addr = find_replacement (&XEXP (addr, 1));
+	  if (GET_CODE (addr) != PLUS)
 	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
-
-	  addr = XEXP (addr, 1);
 	}
 
       if (GET_CODE (addr) == PLUS
@@ -16201,6 +16186,8 @@ rs6000_secondary_reload_inner (rtx reg, rtx mem, r
 	      || !rs6000_legitimate_offset_address_p (PTImode, addr,
 						      false, true)))
 	{
+	  /* find_replacement already recurses into both operands of
+	     PLUS so we don't need to call it here.  */
 	  addr_op1 = XEXP (addr, 0);
 	  addr_op2 = XEXP (addr, 1);
 	  if (!legitimate_indirect_address_p (addr_op1, false))
@@ -16276,7 +16263,7 @@ rs6000_secondary_reload_inner (rtx reg, rtx mem, r
 	      || !VECTOR_MEM_ALTIVEC_P (mode)))
 	{
 	  and_op2 = XEXP (addr, 1);
-	  addr = XEXP (addr, 0);
+	  addr = find_replacement (&XEXP (addr, 0));
 	}
 
       /* If we aren't using a VSX load, save the PRE_MODIFY register and use it
@@ -16288,14 +16275,13 @@ rs6000_secondary_reload_inner (rtx reg, rtx mem, r
 	      || and_op2 != NULL_RTX
 	      || !legitimate_indexed_address_p (XEXP (addr, 1), false)))
 	{
-	  scratch_or_premodify = XEXP (addr, 0);
+	  scratch_or_premodify = find_replacement (&XEXP (addr, 0));
 	  if (!legitimate_indirect_address_p (scratch_or_premodify, false))
 	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
 
-	  if (GET_CODE (XEXP (addr, 1)) != PLUS)
+	  addr = find_replacement (&XEXP (addr, 1));
+	  if (GET_CODE (addr) != PLUS)
 	    rs6000_secondary_reload_fail (__LINE__, reg, mem, scratch, store_p);
-
-	  addr = XEXP (addr, 1);
 	}
 
       if (legitimate_indirect_address_p (addr, false)	/* reg */

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RS6000] power8 internal compiler errors

[David Edelsohn]

On Fri, Feb 14, 2014 at 2:18 AM, Alan Modra <amodra@gmail.com> wrote:
> On Wed, Feb 12, 2014 at 06:47:37PM +0100, Ulrich Weigand wrote:
>> Note that find_replacement itself already recurses into both sides
>> of a PLUS.
>
> Thanks, I missed seeing that.  I'd analysed the bug and knew what
> needed doing from past forays into reload, so went looking for ways to
> get at the reloads, ie. "replacements" at that stage of reload.  Lo
> and behold, there's a function tailor made to do just that!  So I
> plugged in find_replacements() wherever it seemed necessary.
>
>> So it might be
>> easier and cheaper overall to just do a find_replacement within
>> the PRE_MODIFY clause ...
>
> That's a good idea, since PRE_MODIFY doesn't occur that often.
> Here is the revised patch with your recommendations.  Bootstrapped
> and regression tested powerpc64-linux.
>
>         PR target/58675
>         PR target/57935
>         * config/rs6000/rs6000.c (rs6000_secondary_reload_inner): Use
>         find_replacement on parts of insn rtl that might be reloaded.

Okay, this is a cleaner solution.

- David