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

[Bill Schmidt <wschmidt@linux.vnet.ibm.com>]

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
>