Re: Fix PR47976

[Richard Guenther <richard.guenther@gmail.com>]

On Tue, Apr 19, 2011 at 11:32 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Tue, Apr 12, 2011 at 6:18 PM, Jeff Law <law@redhat.com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 04/07/11 04:47, Bernd Schmidt wrote:
>>> PR47976 is a followup to PR47166; the patch there caused this problem.
>>>
>>> The problem occurs in reload. There are two autoinc addresses which
>>> inherit from one another, and we delete an insn that is necessary.
>>>
>>> We reach this code when reloading the second autoinc address:
>>>
>>> 6821        if (optimize && REG_P (oldequiv)
>>> 6822            && REGNO (oldequiv) < FIRST_PSEUDO_REGISTER
>>> 6823            && spill_reg_store[REGNO (oldequiv)]
>>> 6824            && REG_P (old)
>>> (gdb)
>>> 6825            && (dead_or_set_p (insn,
>>> 6826                               spill_reg_stored_to[REGNO (oldequiv)])
>>> 6827                || rtx_equal_p (spill_reg_stored_to[REGNO (oldequiv)],
>>> 6828                                old)))
>>> 6829          delete_output_reload (insn, j, REGNO (oldequiv), reloadreg);
>>>
>>> reload_inherited[j] is 1 at this point, so oldequiv == reloadreg.
>>>
>>> (gdb) p debug_rtx (spill_reg_store[7])
>>> (insn 719 718 232 10 (set (reg:SI 7 r7)
>>>         (reg:SI 3 r3 [orig:339 ivtmp.79 ] [339])) -1 (nil))
>>> (gdb) p debug_rtx (spill_reg_stored_to[7])
>>> (reg:SI 3 r3)
>>>
>>> Prior to the PR47166 patch, we had spill_reg_store[7] equal to insn 718,
>>> which doesn't involve register 7 at all:
>>>
>>> (insn 718 221 719 10 (set (reg:SI 3 r3 [orig:339 ivtmp.79 ] [339])
>>>         (plus:SI (reg:SI 3 r3 [orig:339 ivtmp.79 ] [339])
>>>             (const_int 8 [0x8]))) 4 {*arm_addsi3} (nil))
>>>
>>> That was sufficient to generate enough confusion to make the compiler
>>> think it couldn't delete the output reload.
>>>
>>> I think the problem is simply that the (set (r7) (r3)) is the opposite
>>> direction of a normal spill_reg_store - normally you write a spill reg
>>> to its destination, but autoinc reloads are somewhat special.
>>>
>>> If delete_output_reload isn't valid for (at least some) autoincs, we can
>>> simply not record them in spill_reg_store. That's part of the patch
>>> below; it seems to fix the problem. I've also deleted the code quoted
>>> above since it's pointless to have reload deleting dead stores to
>>> registers: that's what DCE is for. I've observed no code generation
>>> changes other than for the testcase from either of these changes, with
>>> both an ARM and an sh compiler.
>>>
>>> Comments?
>> Looks good to me.  I like letting DCE do its job, particularly if it
>> allows us to even trivially simplify this code ;-)
>
> As you are fine with it and a patch deleting more code than it adds
> always makes me feel comfortable and as I'm trying to get a 4.5.3
> done which is blocked by this bug I will apply the patch after a round
> of testing.
>
> The bugzilla audit trail says the patch tests fine on a few archs,
> I'm going to test x86_64 everywhere and all my available archs
> for a 4.5 branch backport.

Bootstrapped and tested on x86_64-unknown-linux-gnu for trunk
with the set-but-not-used store variable removed.  Installed as
r172706.  I'll wait for some autotester coverage and my own extensive
4.5 testing before doing a 4.6 and 4.5 backport.

Richard.