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
>
