* Fix PR47976
@ 2011-04-07 10:47 Bernd Schmidt
2011-04-12 16:19 ` Jeff Law
0 siblings, 1 reply; 4+ messages in thread
From: Bernd Schmidt @ 2011-04-07 10:47 UTC (permalink / raw)
To: GCC Patches; +Cc: Andrew Stubbs
[-- Attachment #1: Type: text/plain, Size: 2031 bytes --]
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?
Bernd
[-- Attachment #2: pr47976.diff --]
[-- Type: text/plain, Size: 2774 bytes --]
PR fortran/47976
* reload1.c (inc_for_reload): Return void. All callers changed.
(emit_input_reload_insns): Don't try to delete previous output
reloads to a register, or record spill_reg_store for autoincs.
Index: reload1.c
===================================================================
--- reload1.c (revision 170053)
+++ reload1.c (working copy)
@@ -457,7 +457,7 @@ static void emit_reload_insns (struct in
static void delete_output_reload (rtx, int, int, rtx);
static void delete_address_reloads (rtx, rtx);
static void delete_address_reloads_1 (rtx, rtx, rtx);
-static rtx inc_for_reload (rtx, rtx, rtx, int);
+static void inc_for_reload (rtx, rtx, rtx, int);
#ifdef AUTO_INC_DEC
static void add_auto_inc_notes (rtx, rtx);
#endif
@@ -6818,22 +6818,12 @@ emit_input_reload_insns (struct insn_cha
old = XEXP (rl->in_reg, 0);
- if (optimize && REG_P (oldequiv)
- && REGNO (oldequiv) < FIRST_PSEUDO_REGISTER
- && spill_reg_store[REGNO (oldequiv)]
- && REG_P (old)
- && (dead_or_set_p (insn,
- spill_reg_stored_to[REGNO (oldequiv)])
- || rtx_equal_p (spill_reg_stored_to[REGNO (oldequiv)],
- old)))
- delete_output_reload (insn, j, REGNO (oldequiv), reloadreg);
-
/* Prevent normal processing of this reload. */
special = 1;
- /* Output a special code sequence for this case. */
- new_spill_reg_store[REGNO (reloadreg)]
- = inc_for_reload (reloadreg, oldequiv, rl->out,
- rl->inc);
+ /* Output a special code sequence for this case, and forget about
+ spill reg information. */
+ new_spill_reg_store[REGNO (reloadreg)] = NULL;
+ inc_for_reload (reloadreg, oldequiv, rl->out, rl->inc);
}
/* If we are reloading a pseudo-register that was set by the previous
@@ -8643,11 +8633,9 @@ delete_address_reloads_1 (rtx dead_insn,
IN is either identical to VALUE, or some cheaper place to reload from.
INC_AMOUNT is the number to increment or decrement by (always positive).
- This cannot be deduced from VALUE.
-
- Return the instruction that stores into RELOADREG. */
+ This cannot be deduced from VALUE. */
-static rtx
+static void
inc_for_reload (rtx reloadreg, rtx in, rtx value, int inc_amount)
{
/* REG or MEM to be copied and incremented. */
@@ -8707,9 +8695,8 @@ inc_for_reload (rtx reloadreg, rtx in, r
be used as an address. */
if (! post)
- add_insn = emit_insn (gen_move_insn (reloadreg, incloc));
-
- return add_insn;
+ emit_insn (gen_move_insn (reloadreg, incloc));
+ return;
}
}
delete_insns_since (last);
@@ -8745,8 +8732,6 @@ inc_for_reload (rtx reloadreg, rtx in, r
else
emit_insn (gen_sub2_insn (reloadreg, inc));
}
-
- return store;
}
\f
#ifdef AUTO_INC_DEC
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Fix PR47976
2011-04-07 10:47 Fix PR47976 Bernd Schmidt
@ 2011-04-12 16:19 ` Jeff Law
2011-04-19 9:57 ` Richard Guenther
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Law @ 2011-04-12 16:19 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: GCC Patches, Andrew Stubbs
-----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 ;-)
jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iQEcBAEBAgAGBQJNpHtxAAoJEBRtltQi2kC7ytcIAJdW61u1Ugy/56D3mB/J+V8D
FbGgaJSAfdFd2JJm9zCEQUye6VqaQRKdakaH+lCQsuMyFZ0n4/1E3p+4FQnVzUS7
fYrP326TeUZeS0HussNYjA+vINXROgoUyL1OpjU/juIbIZMSkcjPO/v44UmN73iV
CZpcfOBwRsWSLq9PHtgjkR8ySNCU7KkIMjnmo46zoMHLkDWGRjJETlNJx3fVX3A8
wG1WvKKS4HUYhuFwMRh8t4H50CDGty1UpdaJ30skfqvGJvldGrQ9l3twMezTrxCj
rWZiONdZbmYxMZQW90E82+eHh3+wAX/fUwRkeDVIGaNCN5ojkn0TCeFDh9e3l7c=
=PDfh
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Fix PR47976
2011-04-12 16:19 ` Jeff Law
@ 2011-04-19 9:57 ` Richard Guenther
2011-04-19 11:19 ` Richard Guenther
0 siblings, 1 reply; 4+ messages in thread
From: Richard Guenther @ 2011-04-19 9:57 UTC (permalink / raw)
To: Jeff Law; +Cc: Bernd Schmidt, GCC Patches, Andrew Stubbs, Ramana Radhakrishnan
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.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Fix PR47976
2011-04-19 9:57 ` Richard Guenther
@ 2011-04-19 11:19 ` Richard Guenther
0 siblings, 0 replies; 4+ messages in thread
From: Richard Guenther @ 2011-04-19 11:19 UTC (permalink / raw)
To: Jeff Law; +Cc: Bernd Schmidt, GCC Patches, Andrew Stubbs, Ramana Radhakrishnan
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-04-19 10:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-07 10:47 Fix PR47976 Bernd Schmidt
2011-04-12 16:19 ` Jeff Law
2011-04-19 9:57 ` Richard Guenther
2011-04-19 11:19 ` Richard Guenther
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).