Fix twolf -funroll-loops -O3 miscompilation (a semi-latent web.c bug)

Submitted by Paolo Bonzini on Oct. 15, 2012, 1:21 p.m.

Details

Message ID 507C0DCE.4070208@gnu.org
State New
Headers show

Commit Message

Paolo Bonzini Oct. 15, 2012, 1:21 p.m.
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

Comments

Steven Bosscher Oct. 15, 2012, 1:26 p.m.
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
Steven Bosscher Oct. 16, 2012, 10:15 a.m.
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 = &REG_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
Steven Bosscher Oct. 16, 2012, 10:35 a.m.
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 = &REG_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
Paolo Bonzini Oct. 16, 2012, 11:38 a.m.
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 = &REG_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
Steven Bosscher Oct. 16, 2012, 10:54 p.m.
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&regtest 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
Bin.Cheng Oct. 19, 2012, 3:48 a.m.
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&regtest 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.

Patch hide | download patch | download mbox

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