[PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Bill Schmidt]

Hi,

The ada bootstrap failure reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72827
occurs because of a latent bug in the powerpc back end.  The immediate cause is dead store
elimination removing two stores relative to the frame pointer that are not dead; however,
DSE is tricked into doing this because we have temporarily adjusted the frame pointer prior
to performing the loads.  DSE relies on the frame pointer remaining constant to be able to
infer stack stores that are dead.

The code responsible is in rs6000_split_multireg_move, which wants to form new addresses
after reload/lra to use for the individual stores, but has to use only existing hard
registers.  Currently that code always modifies the register holding the base pointer,
which is a bad idea when the base pointer is a register expected to remain fixed.  This
patch identifies those situations and causes the index register to be modified instead.
(If the index register number is zero, indicating constant zero, we fall through to the
existing code; it's ok to add zero to the fixed registers...)

The diffs are a little munged due to indentation and line length adjustments; the change
is just to add the code block labeled with commentary.

We might someday want to rip up this logic and instead change secondary reload to allocate
an extra register to be used during expansion.  But to clear the bootstrap failure, a
simple patch like this seems best.

Bootstrapped and tested on powerpc64le-unknown-linux-gnu with the usual languages plus ada,
with no regressions.  Gnattools now builds properly during bootstrap.

Is this ok for trunk, and eventually for backport to the 5 and 6 branches?

Thanks,
Bill


2016-08-30  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR target/72827
	* config/rs6000/rs6000.c (rs6000_split_multireg_move): Never
	modify the frame pointer or the stack pointer; instead, use
	the index register for temporary address generation.


Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 239871)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -24506,15 +24506,31 @@ rs6000_split_multireg_move (rtx dst, rtx src)
 			      && REG_P (basereg)
 			      && REG_P (offsetreg)
 			      && REGNO (basereg) != REGNO (offsetreg));
-		  if (REGNO (basereg) == 0)
+		  /* We mustn't modify the stack pointer or frame pointer
+		     as this will confuse dead store elimination.  */
+		  if ((REGNO (basereg) == STACK_POINTER_REGNUM
+		       || REGNO (basereg) == HARD_FRAME_POINTER_REGNUM)
+		      && REGNO (offsetreg) != 0)
 		    {
-		      rtx tmp = offsetreg;
-		      offsetreg = basereg;
-		      basereg = tmp;
+		      emit_insn (gen_add3_insn (offsetreg, basereg,
+						offsetreg));
+		      restore_basereg = gen_sub3_insn (offsetreg, offsetreg,
+						       basereg);
+		      dst = replace_equiv_address (dst, offsetreg);
 		    }
-		  emit_insn (gen_add3_insn (basereg, basereg, offsetreg));
-		  restore_basereg = gen_sub3_insn (basereg, basereg, offsetreg);
-		  dst = replace_equiv_address (dst, basereg);
+		  else
+		    {
+		      if (REGNO (basereg) == 0)
+			{
+			  rtx tmp = offsetreg;
+			  offsetreg = basereg;
+			  basereg = tmp;
+			}
+		      emit_insn (gen_add3_insn (basereg, basereg, offsetreg));
+		      restore_basereg = gen_sub3_insn (basereg, basereg,
+						       offsetreg);
+		      dst = replace_equiv_address (dst, basereg);
+		    }
 		}
 	    }
 	  else if (GET_CODE (XEXP (dst, 0)) != LO_SUM)

Re: [PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Segher Boessenkool]

Hi Bill,

On Tue, Aug 30, 2016 at 08:23:46PM -0500, Bill Schmidt wrote:
> The ada bootstrap failure reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72827
> occurs because of a latent bug in the powerpc back end.  The immediate cause is dead store
> elimination removing two stores relative to the frame pointer that are not dead; however,
> DSE is tricked into doing this because we have temporarily adjusted the frame pointer prior
> to performing the loads.  DSE relies on the frame pointer remaining constant to be able to
> infer stack stores that are dead.

DSE should really detect this is happening and not do the wrong thing.
Maybe add an assert somewhere?  Much easier to debug, that way.

> Is this ok for trunk, and eventually for backport to the 5 and 6 branches?

Will it backport without changes?

> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c	(revision 239871)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -24506,15 +24506,31 @@ rs6000_split_multireg_move (rtx dst, rtx src)
>  			      && REG_P (basereg)
>  			      && REG_P (offsetreg)
>  			      && REGNO (basereg) != REGNO (offsetreg));
> -		  if (REGNO (basereg) == 0)
> +		  /* We mustn't modify the stack pointer or frame pointer
> +		     as this will confuse dead store elimination.  */
> +		  if ((REGNO (basereg) == STACK_POINTER_REGNUM
> +		       || REGNO (basereg) == HARD_FRAME_POINTER_REGNUM)
> +		      && REGNO (offsetreg) != 0)

This should only check for HARD_FRAME_POINTER_REGNUM if there *is* a
frame pointer?

>  		    {
> -		      rtx tmp = offsetreg;
> -		      offsetreg = basereg;
> -		      basereg = tmp;

std::swap (basereg, offsetreg);

> +		      emit_insn (gen_add3_insn (offsetreg, basereg,
> +						offsetreg));
> +		      restore_basereg = gen_sub3_insn (offsetreg, offsetreg,
> +						       basereg);
> +		      dst = replace_equiv_address (dst, offsetreg);
>  		    }
> -		  emit_insn (gen_add3_insn (basereg, basereg, offsetreg));
> -		  restore_basereg = gen_sub3_insn (basereg, basereg, offsetreg);
> -		  dst = replace_equiv_address (dst, basereg);
> +		  else
> +		    {
> +		      if (REGNO (basereg) == 0)
> +			{
> +			  rtx tmp = offsetreg;
> +			  offsetreg = basereg;
> +			  basereg = tmp;
> +			}
> +		      emit_insn (gen_add3_insn (basereg, basereg, offsetreg));
> +		      restore_basereg = gen_sub3_insn (basereg, basereg,
> +						       offsetreg);
> +		      dst = replace_equiv_address (dst, basereg);
> +		    }
>  		}
>  	    }
>  	  else if (GET_CODE (XEXP (dst, 0)) != LO_SUM)

If (say) base=r1 offset=r0 this will now adjust r1?  That cannot be good.


Segher

Re: [PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Eric Botcazou]

> DSE should really detect this is happening and not do the wrong thing.
> Maybe add an assert somewhere?  Much easier to debug, that way.

That sounds fragile, functions are allowed to fiddle with the frame pointer in 
the prologue or epilogue (but of course not in the body).  I think that DSE is 
not the only RTL pass which makes this assumption of invariant frame pointer 
in the body, it seems rather fundamental in the RTL middle-end.

-- 
Eric Botcazou

Re: [PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Segher Boessenkool]

On Wed, Aug 31, 2016 at 09:08:37AM +0200, Eric Botcazou wrote:
> > DSE should really detect this is happening and not do the wrong thing.
> > Maybe add an assert somewhere?  Much easier to debug, that way.
> 
> That sounds fragile, functions are allowed to fiddle with the frame pointer in 
> the prologue or epilogue (but of course not in the body).  I think that DSE is 
> not the only RTL pass which makes this assumption of invariant frame pointer 
> in the body, it seems rather fundamental in the RTL middle-end.

Yes exactly, but we do not detect violations of that anywhere it seems.


Segher

Re: [PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Bill Schmidt]

On 8/31/16 1:19 AM, Segher Boessenkool wrote:
> Hi Bill,
>
> On Tue, Aug 30, 2016 at 08:23:46PM -0500, Bill Schmidt wrote:
>> The ada bootstrap failure reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72827
>> occurs because of a latent bug in the powerpc back end.  The immediate cause is dead store
>> elimination removing two stores relative to the frame pointer that are not dead; however,
>> DSE is tricked into doing this because we have temporarily adjusted the frame pointer prior
>> to performing the loads.  DSE relies on the frame pointer remaining constant to be able to
>> infer stack stores that are dead.
> DSE should really detect this is happening and not do the wrong thing.
> Maybe add an assert somewhere?  Much easier to debug, that way.

I'm not sure I'm the right person to do that, as I don't really have any
familiarity with the
DSE code.  I can't even prove to myself that this code is alloca-safe;
it doesn't look like it.
>
>> Is this ok for trunk, and eventually for backport to the 5 and 6 branches?
> Will it backport without changes?

I haven't looked to be sure, but I think only minor changes would be
required.  The bug has
been there since 2010.
>
>> Index: gcc/config/rs6000/rs6000.c
>> ===================================================================
>> --- gcc/config/rs6000/rs6000.c	(revision 239871)
>> +++ gcc/config/rs6000/rs6000.c	(working copy)
>> @@ -24506,15 +24506,31 @@ rs6000_split_multireg_move (rtx dst, rtx src)
>>  			      && REG_P (basereg)
>>  			      && REG_P (offsetreg)
>>  			      && REGNO (basereg) != REGNO (offsetreg));
>> -		  if (REGNO (basereg) == 0)
>> +		  /* We mustn't modify the stack pointer or frame pointer
>> +		     as this will confuse dead store elimination.  */
>> +		  if ((REGNO (basereg) == STACK_POINTER_REGNUM
>> +		       || REGNO (basereg) == HARD_FRAME_POINTER_REGNUM)
>> +		      && REGNO (offsetreg) != 0)
> This should only check for HARD_FRAME_POINTER_REGNUM if there *is* a
> frame pointer?
So, check if hard_frame_pointer_rtx is non-nil?  I can add that.
>
>>  		    {
>> -		      rtx tmp = offsetreg;
>> -		      offsetreg = basereg;
>> -		      basereg = tmp;
> std::swap (basereg, offsetreg);
I didn't actually change that code (which predates the C++ switch), but
sure,
I can make the change.
>
>> +		      emit_insn (gen_add3_insn (offsetreg, basereg,
>> +						offsetreg));
>> +		      restore_basereg = gen_sub3_insn (offsetreg, offsetreg,
>> +						       basereg);
>> +		      dst = replace_equiv_address (dst, offsetreg);
>>  		    }
>> -		  emit_insn (gen_add3_insn (basereg, basereg, offsetreg));
>> -		  restore_basereg = gen_sub3_insn (basereg, basereg, offsetreg);
>> -		  dst = replace_equiv_address (dst, basereg);
>> +		  else
>> +		    {
>> +		      if (REGNO (basereg) == 0)
>> +			{
>> +			  rtx tmp = offsetreg;
>> +			  offsetreg = basereg;
>> +			  basereg = tmp;
>> +			}
>> +		      emit_insn (gen_add3_insn (basereg, basereg, offsetreg));
>> +		      restore_basereg = gen_sub3_insn (basereg, basereg,
>> +						       offsetreg);
>> +		      dst = replace_equiv_address (dst, basereg);
>> +		    }
>>  		}
>>  	    }
>>  	  else if (GET_CODE (XEXP (dst, 0)) != LO_SUM)
> If (say) base=r1 offset=r0 this will now adjust r1?  That cannot be good.
Mm, yeah, that wasn't well-thought.  Was thinking 0, not r0.  Will have
to avoid
that.

Bill
>
>
> Segher
>

Re: [PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Eric Botcazou]

> I'm not sure I'm the right person to do that, as I don't really have any
> familiarity with the DSE code.  I can't even prove to myself that this code
> is alloca-safe; it doesn't look like it.

No FUD, please ;-)  The code is alloca-safe, alloca is about moving the stack 
pointer, not the frame pointer.  The compiler wouldn't be able to compile a 
simple Ada program with optimization enabled if that wasn't the case.

-- 
Eric Botcazou

Re: [PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Bill Schmidt]

> On Aug 31, 2016, at 10:31 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> 
>> I'm not sure I'm the right person to do that, as I don't really have any
>> familiarity with the DSE code.  I can't even prove to myself that this code
>> is alloca-safe; it doesn't look like it.
> 
> No FUD, please ;-)  The code is alloca-safe, alloca is about moving the stack 
> pointer, not the frame pointer.  The compiler wouldn't be able to compile a 
> simple Ada program with optimization enabled if that wasn't the case.


Heh, not my intent. :)  The alloca code was hiding off in cselib anyway...as
I said, I don't have any familiarity here...  

> 
> -- 
> Eric Botcazou
> 

Re: [PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Bill Schmidt]

> On Aug 31, 2016, at 9:00 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
> 
> 
> On 8/31/16 1:19 AM, Segher Boessenkool wrote:
>>> 
>> If (say) base=r1 offset=r0 this will now adjust r1?  That cannot be good.
> Mm, yeah, that wasn't well-thought.  Was thinking 0, not r0.  Will have
> to avoid
> that.

Unfortunately this kind of patch won't be able to address the problem.  If
we have a fixed base register (stack or frame pointer) and the offset register
has been assigned to r0, then the offset register is illegal to use in
replace_equiv_address.  As far as I can see, we can only fix this by making
a change in secondary reload so that we have a scratch register available.
Volunteers? :)

Bill

[PATCH v2, rs6000] Fix PR72827 (ada bootstrap failure)

[Bill Schmidt]

Hi,

After much discussion we've concluded that the first patch isn't
salvageable.  There are register assignments for which we can't fix up
the addressing legally after reload using such tricks (for example:
base=r31, offset=r0).

This patch (suggested by Michael Meissner) instead prevents the problem
by disallowing reg+reg addressing for TImode, allowing D-form addressing
to be used for the separate stores of the GPRs.  This is not an ideal
permanent solution, because it disallows reg+reg addressing not only for
TImode in GPRs, but also for TImode in VSRs.  Our intent is to work on a
solution to provide a scratch register via secondary reload, but this
may take some time.  Thus I'd like to submit this patch as a short-term
solution for the bootstrap problem until then.

I plan to do a performance evaluation of this patch for informational
purposes, but I don't expect much change, and the results won't affect
our choice to handle this properly in secondary reload.

Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
regressions, and the gnattools now build properly so that the ada
bootstrap succeeds.  Is this ok for trunk?

Thanks,
Bill


2016-08-31  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
	    Michael Meissner <meissner@linux.vnet.ibm.com>

	PR target/72827
	* config/rs6000/rs6000.c (rs6000_legitimize_address): Avoid
	reg+reg addressing for TImode.
	(rs6000_legitimate_address_p): Only allow register indirect
	addressing for TImode, even without TARGET_QUAD_MEMORY.


Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 239871)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -8409,7 +8409,7 @@ rs6000_legitimize_address (rtx x, rtx oldx ATTRIBU
 	 pointer, so it works with both GPRs and VSX registers.  */
       /* Make sure both operands are registers.  */
       else if (GET_CODE (x) == PLUS
-	       && (mode != TImode || !TARGET_QUAD_MEMORY))
+	       && (mode != TImode || !TARGET_VSX_TIMODE))
 	return gen_rtx_PLUS (Pmode,
 			     force_reg (Pmode, XEXP (x, 0)),
 			     force_reg (Pmode, XEXP (x, 1)));
@@ -9418,12 +9418,16 @@ rs6000_legitimate_address_p (machine_mode mode, rt
 	return 1;
     }
 
-  /* For TImode, if we have load/store quad and TImode in VSX registers, only
-     allow register indirect addresses.  This will allow the values to go in
-     either GPRs or VSX registers without reloading.  The vector types would
-     tend to go into VSX registers, so we allow REG+REG, while TImode seems
+  /* For TImode, if we have TImode in VSX registers, only allow register
+     indirect addresses.  This will allow the values to go in either GPRs
+     or VSX registers without reloading.  The vector types would tend to
+     go into VSX registers, so we allow REG+REG, while TImode seems
      somewhat split, in that some uses are GPR based, and some VSX based.  */
-  if (mode == TImode && TARGET_QUAD_MEMORY && TARGET_VSX_TIMODE)
+  /* FIXME: We could loosen this by changing the following to
+       if (mode == TImode && TARGET_QUAD_MEMORY && TARGET_VSX_TIMODE)
+     but currently we cannot allow REG+REG addressing for TImode.  See
+     PR72827 for complete details on how this ends up hoodwinking DSE.  */
+  if (mode == TImode && TARGET_VSX_TIMODE)
     return 0;
   /* If not REG_OK_STRICT (before reload) let pass any stack offset.  */
   if (! reg_ok_strict

Re: [PATCH v2, rs6000] Fix PR72827 (ada bootstrap failure)

[Eric Botcazou]

> This patch (suggested by Michael Meissner) instead prevents the problem
> by disallowing reg+reg addressing for TImode, allowing D-form addressing
> to be used for the separate stores of the GPRs.  This is not an ideal
> permanent solution, because it disallows reg+reg addressing not only for
> TImode in GPRs, but also for TImode in VSRs.  Our intent is to work on a
> solution to provide a scratch register via secondary reload, but this
> may take some time.  Thus I'd like to submit this patch as a short-term
> solution for the bootstrap problem until then.

Thanks for working on the problem and devising a fix!

-- 
Eric Botcazou

Re: [PATCH v2, rs6000] Fix PR72827 (ada bootstrap failure)

[Segher Boessenkool]

On Wed, Aug 31, 2016 at 09:15:32PM -0500, Bill Schmidt wrote:
> This patch (suggested by Michael Meissner) instead prevents the problem
> by disallowing reg+reg addressing for TImode, allowing D-form addressing
> to be used for the separate stores of the GPRs.  This is not an ideal
> permanent solution, because it disallows reg+reg addressing not only for
> TImode in GPRs, but also for TImode in VSRs.  Our intent is to work on a
> solution to provide a scratch register via secondary reload, but this
> may take some time.  Thus I'd like to submit this patch as a short-term
> solution for the bootstrap problem until then.
> 
> I plan to do a performance evaluation of this patch for informational
> purposes, but I don't expect much change, and the results won't affect
> our choice to handle this properly in secondary reload.
> 
> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
> regressions, and the gnattools now build properly so that the ada
> bootstrap succeeds.  Is this ok for trunk?

Yes please.  Thanks,


Segher

Re: [PATCH v2, rs6000] Fix PR72827 (ada bootstrap failure)

[Bill Schmidt]

On Sep 1, 2016, at 9:32 AM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> 
> On Wed, Aug 31, 2016 at 09:15:32PM -0500, Bill Schmidt wrote:
>> This patch (suggested by Michael Meissner) instead prevents the problem
>> by disallowing reg+reg addressing for TImode, allowing D-form addressing
>> to be used for the separate stores of the GPRs.  This is not an ideal
>> permanent solution, because it disallows reg+reg addressing not only for
>> TImode in GPRs, but also for TImode in VSRs.  Our intent is to work on a
>> solution to provide a scratch register via secondary reload, but this
>> may take some time.  Thus I'd like to submit this patch as a short-term
>> solution for the bootstrap problem until then.
>> 
>> I plan to do a performance evaluation of this patch for informational
>> purposes, but I don't expect much change, and the results won't affect
>> our choice to handle this properly in secondary reload.
>> 
>> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
>> regressions, and the gnattools now build properly so that the ada
>> bootstrap succeeds.  Is this ok for trunk?
> 
> Yes please.  Thanks,

r239938, thanks!

Bill
> 
> 
> Segher
> 

Re: [PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Jeff Law]

On 08/31/2016 01:08 AM, Eric Botcazou wrote:
>> DSE should really detect this is happening and not do the wrong thing.
>> Maybe add an assert somewhere?  Much easier to debug, that way.
>
> That sounds fragile, functions are allowed to fiddle with the frame pointer in
> the prologue or epilogue (but of course not in the body).  I think that DSE is
> not the only RTL pass which makes this assumption of invariant frame pointer
> in the body, it seems rather fundamental in the RTL middle-end.
That's my recollection as well -- I recall many patches flying by 
through the years that assumed the frame pointer was invariant -- but 
they were mostly (all?) in things that ran before we add the 
prologue/epilogue to the INSN chain.

jeff

Re: [PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Segher Boessenkool]

On Tue, Sep 06, 2016 at 03:24:24PM -0600, Jeff Law wrote:
> On 08/31/2016 01:08 AM, Eric Botcazou wrote:
> >>DSE should really detect this is happening and not do the wrong thing.
> >>Maybe add an assert somewhere?  Much easier to debug, that way.
> >
> >That sounds fragile, functions are allowed to fiddle with the frame 
> >pointer in
> >the prologue or epilogue (but of course not in the body).  I think that 
> >DSE is
> >not the only RTL pass which makes this assumption of invariant frame 
> >pointer
> >in the body, it seems rather fundamental in the RTL middle-end.
> That's my recollection as well -- I recall many patches flying by 
> through the years that assumed the frame pointer was invariant -- but 
> they were mostly (all?) in things that ran before we add the 
> prologue/epilogue to the INSN chain.

We could simply check if the frame pointer (or stack pointer) is changed
while RTX_FRAME_RELATED_P is not set?  Can that ever happen on "proper"
code?


Segher

Re: [PATCH, rs6000] Fix PR72827 (ada bootstrap failure)

[Jeff Law]

On 09/06/2016 04:14 PM, Segher Boessenkool wrote:
> On Tue, Sep 06, 2016 at 03:24:24PM -0600, Jeff Law wrote:
>> On 08/31/2016 01:08 AM, Eric Botcazou wrote:
>>>> DSE should really detect this is happening and not do the wrong thing.
>>>> Maybe add an assert somewhere?  Much easier to debug, that way.
>>>
>>> That sounds fragile, functions are allowed to fiddle with the frame
>>> pointer in
>>> the prologue or epilogue (but of course not in the body).  I think that
>>> DSE is
>>> not the only RTL pass which makes this assumption of invariant frame
>>> pointer
>>> in the body, it seems rather fundamental in the RTL middle-end.
>> That's my recollection as well -- I recall many patches flying by
>> through the years that assumed the frame pointer was invariant -- but
>> they were mostly (all?) in things that ran before we add the
>> prologue/epilogue to the INSN chain.
>
> We could simply check if the frame pointer (or stack pointer) is changed
> while RTX_FRAME_RELATED_P is not set?  Can that ever happen on "proper"
> code?
Worth a try, though I'm not convinced we're setting RTX_FRAME_RELATED_P 
correctly, particularly for the legacy targets.
jeff