Message ID | 4D9D963B.7070001@codesourcery.com |
---|---|
State | New |
Headers | show |
-----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-----
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.
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.
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; } #ifdef AUTO_INC_DEC