Bug 45051 - [4.6 Regression]: gcc.c-torture/execute/builtins/abs-2.c and abs-3.c due to "track subwords of DImode allocnos"
Summary: [4.6 Regression]: gcc.c-torture/execute/builtins/abs-2.c and abs-3.c due to "...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.6.0
: P3 normal
Target Milestone: 4.6.0
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks: 41085 47166
  Show dependency treegraph
 
Reported: 2010-07-24 02:42 UTC by Hans-Peter Nilsson
Modified: 2011-01-04 11:52 UTC (History)
3 users (show)

See Also:
Host: x86_64-unknown-linux-gnu
Target: cris-axis-elf
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-07-24 03:02:58


Attachments
Shortened gcc.c-torture/execute/builtins/abs-2.c (208 bytes, text/plain)
2010-07-24 02:46 UTC, Hans-Peter Nilsson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hans-Peter Nilsson 2010-07-24 02:42:50 UTC
With revision 162417 this test passed.
From revision 162418 and on, these tests (or "this test" as abs-3.c is effectively the same as abs-2.c) has failed as follows:
Running /tmp/badabs/gcc/gcc/testsuite/gcc.c-torture/execute/builtins/builtins.exp ...
FAIL: gcc.c-torture/execute/builtins/abs-2.c execution,  -O2 
FAIL: gcc.c-torture/execute/builtins/abs-2.c execution,  -O3 -fomit-frame-pointer 
FAIL: gcc.c-torture/execute/builtins/abs-2.c execution,  -O3 -g 
FAIL: gcc.c-torture/execute/builtins/abs-2.c execution,  -Os 
FAIL: gcc.c-torture/execute/builtins/abs-2.c execution,  -O2 -flto 
FAIL: gcc.c-torture/execute/builtins/abs-2.c execution,  -O2 -fwhopr 
FAIL: gcc.c-torture/execute/builtins/abs-3.c execution,  -O2 
FAIL: gcc.c-torture/execute/builtins/abs-3.c execution,  -O3 -fomit-frame-pointer 
FAIL: gcc.c-torture/execute/builtins/abs-3.c execution,  -O3 -g 
FAIL: gcc.c-torture/execute/builtins/abs-3.c execution,  -Os 
FAIL: gcc.c-torture/execute/builtins/abs-3.c execution,  -O2 -flto 
FAIL: gcc.c-torture/execute/builtins/abs-3.c execution,  -O2 -fwhopr 

With the message in the logfile being:
...
PASS: gcc.c-torture/execute/builtins/abs-2.c compilation,  -O2 
program stopped with signal 6.
FAIL: gcc.c-torture/execute/builtins/abs-2.c execution,  -O2 
...
(i.e. abort being called)

Author of patch in suspect revision range CC:ed.

At a glance, the code change causes a register containing 0 to be used as containing 0xffffffff for the low-part of 9223372036854775807LL ((1<<63)-1), i.e. a mis-tracking of a DImode subreg.

I'll attach a smaller test-case and an assembly diff pointing out the wrong code.
Comment 1 Hans-Peter Nilsson 2010-07-24 02:46:20 UTC
Created attachment 21295 [details]
Shortened gcc.c-torture/execute/builtins/abs-2.c

Compile with e.g. -O2
Comment 2 Hans-Peter Nilsson 2010-07-24 02:51:28 UTC
(In reply to comment #0)
Oops; replace:
> containing 0xffffffff for the low-part of 9223372036854775807LL ((1<<63)-1),
with:
> containing 0x7fffffff for the high-part of 9223372036854775807LL ((1<<63)-1),


Comment 3 Hans-Peter Nilsson 2010-07-24 03:02:58 UTC
--- good:r162417/abs-2.s        Sat Jul 24 03:13:36 2010
+++ bad:r162418/abs-2.s Sat Jul 24 03:15:53 2010
@@ -9,27 +9,27 @@ _main_test:
        move $srp,[$sp]
        subu.b 84,$sp
        movem $r8,[$sp]
-       clear.d $r4
-       clear.d $r5
-       move.b 1,$r4
-       move.d $r4,[$sp+76]
-       move.d $r5,[$sp+80]
+       clear.d $r0      ; (set (reg:SI r0) (const_int 0))
+       clear.d $r1
+       move.b 1,$r0     ; (set (strict_low_part:QI (reg:SI r0)) (const_int 1))
+       move.d $r0,[$sp+76]
+       move.d $r1,[$sp+80]
        moveq -1,$r12
        move.d $r12,[$sp+68]
        move.d $r12,[$sp+72]
-       clear.d $r10
-       move.d $r4,$r11
-       move.d $r11,$r8
-       move.d -2147483648,$r0
-       move.d $r11,[$sp+60]
-       move.d $r10,[$sp+64]
-       move.d 2147483647,$r11
+       move.d $r0,$r10
+       move.d -2147483648,$r11
+       move.d $r10,[$sp+60]
+       move.d $r11,[$sp+64]
+       clear.b $r0     ; (set (strict_low_part:QI (reg:SI r9)) (const_int 0))
+       move.d $r0,$r9  ; (set (reg:SI r9) (reg:SI r0))
+       move.d 2147483647,$r0
        move.d $r12,[$sp+52]
-       move.d $r11,[$sp+56]
-       move.d $r8,[$sp+44]
-       move.d $r0,[$sp+48]
+       move.d $r9,[$sp+56]     ; stored here
+       move.d $r10,[$sp+44]
+       move.d $r11,[$sp+48]
        move.d $r12,[$sp+36]
-       move.d $r11,[$sp+40]
+       move.d $r0,[$sp+40]
        move.d [$sp+52],$r12
        move.d [$sp+56],$r13    ; loaded here
        test.d $r12 ; fatally tested below; containing 0, expected 0x7fffffff
Comment 4 Bernd Schmidt 2010-07-27 09:35:06 UTC
Subject: Bug 45051

Author: bernds
Date: Tue Jul 27 09:34:51 2010
New Revision: 162558

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162558
Log:
	PR rtl-optimization/45051
	* reload1.c (delete_output_reload): Use refers_to_regno_p rather
	than reg_mentioned_p.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/reload1.c

Comment 5 Bernd Schmidt 2010-07-27 21:49:07 UTC
Assuming fixed and closing.  Please reopen if you still have a problem.
Comment 6 Hans-Peter Nilsson 2010-09-21 21:26:19 UTC
Subject: Bug 45051

Author: hp
Date: Tue Sep 21 21:25:57 2010
New Revision: 164498

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=164498
Log:
	PR rtl-optimization/41085
	Backport from mainline
	2010-07-27  Bernd Schmidt  <bernds@codesourcery.com>

	PR rtl-optimization/45051
	* reload1.c (delete_output_reload): Use refers_to_regno_p rather
	than reg_mentioned_p.

Modified:
    branches/gcc-4_5-branch/gcc/ChangeLog
    branches/gcc-4_5-branch/gcc/reload1.c

Comment 7 Ian Bolton 2010-12-22 16:23:21 UTC
(In reply to comment #5)
> Assuming fixed and closing.  Please reopen if you still have a problem.

This patch has caused SpecCPU2000 Ammp to fail for ARM -O3 thumb.

I assume the patch was meant to prevent deletions that shouldn't occur.  This might be what happens for the original symptomatic test, but I am now seeing extra deletions that shouldn't happen for Ammp.

For example, without this patch, you get these insns somewhere in the ira dump for mm_fv_update_nonbon() from rectmm.c:

 (insn 3163 3161 3164 107 rectmm.c:1041 (set (reg:SI 1 r1) 
    (plus:SI (reg:SI 1 r1) 
       (const_int 280 [0x118]))) 4 {*arm_addsi3} (nil))

 (insn 3164 3163 1730 107 rectmm.c:1041 (set (reg:SI 3 r3) 
    (reg:SI 1 r1)) 586 {*thumb2_movsi_vfp} (nil))

With the patch, you lose the add and just get this:

 (insn 3164 3161 1730 107 rectmm.c:1041 (set (reg:SI 3 r3)
    (reg:SI 1 r1)) 586 {*thumb2_movsi_vfp} (nil)) 

The incrementing of r1 is perfectly legitimate and useful and removing it is a bug.  Other increments of r9, ip, r0 and r3 are also lost.

I think the issue might be that reg_mentioned_p() considers output registers to have been "mentioned", whereas the refers_to_regno_p() does not consider an output register to have been "referred to".  I can see the problem with only using reg_mentioned_p() because it doesn't handle subregs, but there is also a problem with only using refers_to_regno_p(), as we can see with this segfault in Ammp.

I therefore wonder if the fix might be this:

Index: gcc/reload1.c
===================================================================
--- gcc/reload1.c       (revision 168082)
+++ gcc/reload1.c       (working copy)
@@ -8395,7 +8395,8 @@ delete_output_reload (rtx insn, int j, i
       if (NOTE_INSN_BASIC_BLOCK_P (i1))
        return;
       if ((NONJUMP_INSN_P (i1) || CALL_P (i1))
-         && refers_to_regno_p (regno, regno + nregs, PATTERN (i1), NULL))
+         && (refers_to_regno_p (regno, regno + nregs, PATTERN (i1), NULL)
+             || reg_mentioned_p (reg, PATTERN (i1))))
        {
          /* If this is USE in front of INSN, we only have to check that
             there are no more references than accounted for by inheritance.  */

I am heading off for Christmas vacation shortly, so cannot look into this any further, but I wanted to record my findings so far publicly.  Apologies if there is missing information.  I return to work Jan 4th.
Comment 8 Hans-Peter Nilsson 2010-12-23 01:56:08 UTC
(In reply to comment #7)
>  (insn 3163 3161 3164 107 rectmm.c:1041 (set (reg:SI 1 r1) 
>     (plus:SI (reg:SI 1 r1) 
>        (const_int 280 [0x118]))) 4 {*arm_addsi3} (nil))

> I think the issue might be that reg_mentioned_p() considers output registers to
> have been "mentioned", whereas the refers_to_regno_p() does not consider an
> output register to have been "referred to".

But r1 is an input as well as an output , i.e. "referred to", so insn 3163 should have matched without your patch.  I'm missing analysis on why that didn't happen.  Nevertheless, I'm testing r162418 with your patch.

Also, you should open a separate PR for the regression you see, not just reuse this one. Use the depends-on features for the dependency.
Comment 9 Hans-Peter Nilsson 2010-12-23 14:29:13 UTC
(In reply to comment #8)
> Nevertheless, I'm testing r162418 with your patch.

FWIW, no regressions for cris-elf, with your patch (manually) applied on top of Bernds's patch applied to r162418.
Comment 10 Richard Biener 2011-01-03 20:01:41 UTC
Original problem was fixed.  Please open a new bug for any other issues.