| Message ID | 507C0DCE.4070208@gnu.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 15, 2012 at 3:21 PM, Paolo Bonzini <bonzini@gnu.org> wrote: > Il 15/10/2012 14:53, Steven Bosscher ha scritto: >> I think I've shown above that we're all looking at the wrong pass... > > I think you have... so we want a patch like this? I don't think so. df_kill_notes is already supposed to take care of this. Ciao! Steven
On Mon, Oct 15, 2012 at 3:26 PM, Steven Bosscher wrote: > On Mon, Oct 15, 2012 at 3:21 PM, Paolo Bonzini wrote: >> Il 15/10/2012 14:53, Steven Bosscher ha scritto: >>> I think I've shown above that we're all looking at the wrong pass... >> >> I think you have... so we want a patch like this? > > I don't think so. df_kill_notes is already supposed to take care of this. But it doesn't because if the SET_DEST of an insn is the same as the register dieing in the insn's REG_EQUAL note, the reg is live at the end of the insn and so the note stays: Breakpoint 2, df_kill_notes (insn=0x7ffff5e3e7e0, live=0x7fffffffda90) at ../../trunk/gcc/df-problems.c:2833 2833 rtx *pprev = ®_NOTES (insn); 1: debug_rtx(insn) = (insn 79 50 52 8 (set (reg:DI 72 [ ivtmp.17D.1758 ]) (reg:DI 103 [ ivtmp.17D.1758 ])) -1 (expr_list:REG_DEAD (reg:DI 103 [ ivtmp.17D.1758 ]) (expr_list:REG_EQUAL (plus:DI (reg:DI 72 [ ivtmp.17D.1758 ]) (const_int 24 [0x18])) (nil)))) void (gdb) undisp 1 (gdb) p debug_bitmap(live) first = 0x1627200 current = 0x1627200 indx = 0 0x1627200 next = (nil) prev = (nil) indx = 0 bits = { 6 7 16 20 72 82 85 87 } $2 = void (gdb) So GCC should be looking at whether the reg in the REG_EQUAL note is dead *before* the insn. Bottom line: This is a bug in df_kill_notes. Ciao! Steven
On Tue, Oct 16, 2012 at 12:15 PM, Steven Bosscher wrote: > On Mon, Oct 15, 2012 at 3:26 PM, Steven Bosscher wrote: >> On Mon, Oct 15, 2012 at 3:21 PM, Paolo Bonzini wrote: >>> Il 15/10/2012 14:53, Steven Bosscher ha scritto: >>>> I think I've shown above that we're all looking at the wrong pass... >>> >>> I think you have... so we want a patch like this? >> >> I don't think so. df_kill_notes is already supposed to take care of this. > > But it doesn't because if the SET_DEST of an insn is the same as the > register dieing in the insn's REG_EQUAL note, the reg is live at the > end of the insn and so the note stays: > > Breakpoint 2, df_kill_notes (insn=0x7ffff5e3e7e0, live=0x7fffffffda90) > at ../../trunk/gcc/df-problems.c:2833 > 2833 rtx *pprev = ®_NOTES (insn); > 1: debug_rtx(insn) = (insn 79 50 52 8 (set (reg:DI 72 [ ivtmp.17D.1758 ]) > (reg:DI 103 [ ivtmp.17D.1758 ])) -1 > (expr_list:REG_DEAD (reg:DI 103 [ ivtmp.17D.1758 ]) > (expr_list:REG_EQUAL (plus:DI (reg:DI 72 [ ivtmp.17D.1758 ]) > (const_int 24 [0x18])) > (nil)))) > void > (gdb) undisp 1 > (gdb) p debug_bitmap(live) > > first = 0x1627200 current = 0x1627200 indx = 0 > 0x1627200 next = (nil) prev = (nil) indx = 0 > bits = { 6 7 16 20 72 82 85 87 } > $2 = void > (gdb) > > > So GCC should be looking at whether the reg in the REG_EQUAL note is > dead *before* the insn. > > Bottom line: This is a bug in df_kill_notes. I think this should fix it. Can't test it right now, so help appreciated (Honza: hint hint! ;-) Ciao! Steven
Il 16/10/2012 12:35, Steven Bosscher ha scritto: > On Tue, Oct 16, 2012 at 12:15 PM, Steven Bosscher wrote: >> On Mon, Oct 15, 2012 at 3:26 PM, Steven Bosscher wrote: >>> On Mon, Oct 15, 2012 at 3:21 PM, Paolo Bonzini wrote: >>>> Il 15/10/2012 14:53, Steven Bosscher ha scritto: >>>>> I think I've shown above that we're all looking at the wrong pass... >>>> >>>> I think you have... so we want a patch like this? >>> >>> I don't think so. df_kill_notes is already supposed to take care of this. >> >> But it doesn't because if the SET_DEST of an insn is the same as the >> register dieing in the insn's REG_EQUAL note, the reg is live at the >> end of the insn and so the note stays: >> >> Breakpoint 2, df_kill_notes (insn=0x7ffff5e3e7e0, live=0x7fffffffda90) >> at ../../trunk/gcc/df-problems.c:2833 >> 2833 rtx *pprev = ®_NOTES (insn); >> 1: debug_rtx(insn) = (insn 79 50 52 8 (set (reg:DI 72 [ ivtmp.17D.1758 ]) >> (reg:DI 103 [ ivtmp.17D.1758 ])) -1 >> (expr_list:REG_DEAD (reg:DI 103 [ ivtmp.17D.1758 ]) >> (expr_list:REG_EQUAL (plus:DI (reg:DI 72 [ ivtmp.17D.1758 ]) >> (const_int 24 [0x18])) >> (nil)))) >> void >> (gdb) undisp 1 >> (gdb) p debug_bitmap(live) >> >> first = 0x1627200 current = 0x1627200 indx = 0 >> 0x1627200 next = (nil) prev = (nil) indx = 0 >> bits = { 6 7 16 20 72 82 85 87 } >> $2 = void >> (gdb) >> >> >> So GCC should be looking at whether the reg in the REG_EQUAL note is >> dead *before* the insn. >> >> Bottom line: This is a bug in df_kill_notes. Yep, and it could cause wrong code generation, couldn't it? Because the new (reg:DI 72) is obviously not equal to (plus:DI (reg:DI 72) (const_int 24)), but it could be propagated to subsequent insns. So I think this patch should be backported to all release branches after regstrapping. > I think this should fix it. Can't test it right now, so help > appreciated (Honza: hint hint! ;-) Ok after bootstrap, regtest and checking that it actually fixes the bug. Paolo
On Tue, Oct 16, 2012 at 1:38 PM, Paolo Bonzini wrote: >>> Bottom line: This is a bug in df_kill_notes. > > Yep, and it could cause wrong code generation, couldn't it? Because the > new (reg:DI 72) is obviously not equal to (plus:DI (reg:DI 72) > (const_int 24)), but it could be propagated to subsequent insns. Right. > So I think this patch should be backported to all release branches after > regstrapping. Agreed, but let's wait a few days at least to see what happens with the patch on the trunk. > Ok after bootstrap, regtest and checking that it actually fixes the bug. I made sure of that before posting the patch ;-) Bootstrap®test is running on x86_64-unknown-linux-gnu and powerpc64-unknown-linux-gnu. I'll commit the patch tomorrow if there are no test suite regressions. Well done to us all for probing until we found the root cause of this bug! Ciao! Steven
On Wed, Oct 17, 2012 at 6:54 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Tue, Oct 16, 2012 at 1:38 PM, Paolo Bonzini wrote: >>>> Bottom line: This is a bug in df_kill_notes. >> >> Yep, and it could cause wrong code generation, couldn't it? Because the >> new (reg:DI 72) is obviously not equal to (plus:DI (reg:DI 72) >> (const_int 24)), but it could be propagated to subsequent insns. > > Right. > > >> So I think this patch should be backported to all release branches after >> regstrapping. > > Agreed, but let's wait a few days at least to see what happens with > the patch on the trunk. > > >> Ok after bootstrap, regtest and checking that it actually fixes the bug. > > I made sure of that before posting the patch ;-) > > Bootstrap®test is running on x86_64-unknown-linux-gnu and > powerpc64-unknown-linux-gnu. I'll commit the patch tomorrow if there > are no test suite regressions. > > Well done to us all for probing until we found the root cause of this bug! > Hi Jan, For the test case: main() { configure2(); } Please make main return ZERO by default. When cross testing with qemu/simulator, the wrap exit checks whether the case by verifying the return value. For ARM target, R0 is checked while it may be clobbered(thus non-ZERO) if main does not return any value, which causes failure of test case. Thanks very much.
Index: df-problems.c =================================================================== --- df-problems.c (revisione 183719) +++ df-problems.c (copia locale) @@ -3480,6 +3485,18 @@ df_note_bb_compute (unsigned int bb_inde } } + for (use_rec = DF_INSN_UID_EQ_USES (uid); *use_rec; use_rec++) + { + df_ref use = *use_rec; + unsigned int uregno = DF_REF_REGNO (use); + + if (!bitmap_bit_p (live, uregno)) + { + remove_note (insn, find_reg_equal_equiv_note (insn)); + break; + } + } + if (debug_insn == -1) { /* ??? We could probably do better here, replacing dead
Il 15/10/2012 14:53, Steven Bosscher ha scritto: > I think I've shown above that we're all looking at the wrong pass... I think you have... so we want a patch like this? Paolo