On Tue, Apr 12, 2011 at 6:18 PM, Jeff Law <l...@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.
Richard.
