This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Fix PR47976
- From: Richard Guenther <richard dot guenther at gmail dot com>
- To: Jeff Law <law at redhat dot com>
- Cc: Bernd Schmidt <bernds at codesourcery dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, Andrew Stubbs <ams at codesourcery dot com>, Ramana Radhakrishnan <ramana dot gcc at googlemail dot com>
- Date: Tue, 19 Apr 2011 11:32:08 +0200
- Subject: Re: Fix PR47976
- References: <4D9D963B.7070001@codesourcery.com> <4DA47B72.7010005@redhat.com>
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.
Richard.