Bug 19296 - [3.3/3.4/4.0 regression] Range check on short miscompiled at -O
Summary: [3.3/3.4/4.0 regression] Range check on short miscompiled at -O
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 3.3.5
: P2 critical
Target Milestone: 3.3.6
Assignee: Eric Botcazou
URL:
Keywords: patch, wrong-code
Depends on:
Blocks: 23241
  Show dependency tree / graph
 
Reported: 2005-01-06 19:18 UTC by Falk Hueffner
Modified: 2005-08-05 04:47 UTC (History)
2 users (show)

See Also:
Host:
Target: i686-linux
Build:
Known to work: 2.95.3
Known to fail: 3.4.0 3.4.4 4.0.0 3.0.4 3.2.3 3.3.3 3.3.6
Last reconfirmed: 2005-01-06 20:37:10

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Falk Hueffner 2005-01-06 19:18:11 UTC 
[forwarded from http://bugs.debian.org/288721]

This fails with 3.3.5 at -O and higher. Regression from 2.95. Also doesn't fail
on MIPS or Alpha. Debian 3.4.4 20041218 is OK.

void abort(void);
__attribute__((noinline)) void f(unsigned short ad) {
    if (ad >= 0x4000 && ad < 0xc000) 
        abort();
}

int main(void) {
    f(0xff00); 
    return 0;
}
Comment 1 Andrew Pinski 2005-01-06 19:40:30 UTC 
It also fails with last nights build of gcc of the mainline.
And 3.4.0, 3.3.3, 3.2.3 and 3.0.4.

Here is a testcase which also fails too (this is what fold does):
void abort ();
void f1(unsigned short ad)
{
        if ((short int) (ad - 16384) >= 0)
                abort ();
}
int main (void)
{
        f1(0xff00);
        return 0;
}

The RTL is wrong right away:
(insn 13 11 14 (set (reg:HI 60)
        (subreg/u:HI (reg/v:SI 58 [ ad ]) 0)) -1 (nil)
    (nil))

(insn 14 13 15 (parallel [
            (set (reg:SI 61)
                (plus:SI (subreg:SI (reg:HI 60) 0)
                    (const_int -16384 [0xffffc000])))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil)
    (nil))

(insn 15 14 16 (set (reg:CCGOC 17 flags)
        (compare:CCGOC (subreg:HI (reg:SI 61) 0)
            (const_int 0 [0x0]))) -1 (nil)
    (nil))

(jump_insn 16 15 0 (set (pc)
        (if_then_else (lt (reg:CCGOC 17 flags)
                (const_int 0 [0x0]))
            (label_ref 0)
            (pc))) -1 (nil)
    (expr_list:REG_BR_PROB (const_int 9900 [0x26ac])
        (nil)))


on PPC we get:
(insn 11 9 12 (set (reg:HI 119)
        (subreg/u:HI (reg/v:SI 118 [ ad ]) 2)) -1 (nil)
    (nil))

(insn 12 11 13 (set (reg:SI 120)
        (plus:SI (subreg:SI (reg:HI 119) 0)
            (const_int -16384 [0xffffffffffffc000]))) -1 (nil)
    (nil))

(insn 13 12 14 (set (reg:SI 121)
        (sign_extend:SI (subreg:HI (reg:SI 120) 2))) -1 (nil)
    (nil))

(insn 14 13 15 (set (reg:CC 122)
        (compare:CC (reg:SI 121)
            (const_int 0 [0x0]))) -1 (nil)
    (nil))

(jump_insn 15 14 0 (set (pc)
        (if_then_else (lt (reg:CC 122)
                (const_int 0 [0x0]))
            (label_ref 0)
            (pc))) -1 (nil)
    (expr_list:REG_BR_PROB (const_int 9900 [0x26ac])
        (nil)))


Notice the sign_extend in the PPC but not in the x86 case.
Comment 2 Eric Botcazou 2005-01-06 20:37:10 UTC 
It only fails on i686, neither on i386 nor i586.
Comment 3 Eric Botcazou 2005-01-06 20:56:47 UTC 
Investigating.
Comment 4 Eric Botcazou 2005-01-06 22:24:57 UTC 
 > The RTL is wrong right away:
> (insn 13 11 14 (set (reg:HI 60)
>         (subreg/u:HI (reg/v:SI 58 [ ad ]) 0)) -1 (nil)
>     (nil))
> 
> (insn 14 13 15 (parallel [
>             (set (reg:SI 61)
>                 (plus:SI (subreg:SI (reg:HI 60) 0)
>                     (const_int -16384 [0xffffc000])))
>             (clobber (reg:CC 17 flags))
>         ]) -1 (nil)
>     (nil))
> 
> (insn 15 14 16 (set (reg:CCGOC 17 flags)
>         (compare:CCGOC (subreg:HI (reg:SI 61) 0)
>             (const_int 0 [0x0]))) -1 (nil)
>     (nil))
> 
> (jump_insn 16 15 0 (set (pc)
>         (if_then_else (lt (reg:CCGOC 17 flags)
>                 (const_int 0 [0x0]))
>             (label_ref 0)
>             (pc))) -1 (nil)
>     (expr_list:REG_BR_PROB (const_int 9900 [0x26ac])
>         (nil)))

I don't think the initial RTL is wrong: note that, while we perform the addition
in SImode, we still compare in HImode.  The problem appears to come from the
combiner, which gets rid of the HImode in the comparison.

> on PPC we get:
> (insn 11 9 12 (set (reg:HI 119)
>         (subreg/u:HI (reg/v:SI 118 [ ad ]) 2)) -1 (nil)
>     (nil))
> 
> (insn 12 11 13 (set (reg:SI 120)
>         (plus:SI (subreg:SI (reg:HI 119) 0)
>             (const_int -16384 [0xffffffffffffc000]))) -1 (nil)
>     (nil))
> 
> (insn 13 12 14 (set (reg:SI 121)
>         (sign_extend:SI (subreg:HI (reg:SI 120) 2))) -1 (nil)
>     (nil))
> 
> (insn 14 13 15 (set (reg:CC 122)
>         (compare:CC (reg:SI 121)
>             (const_int 0 [0x0]))) -1 (nil)
>     (nil))
> 
> (jump_insn 15 14 0 (set (pc)
>         (if_then_else (lt (reg:CC 122)
>                 (const_int 0 [0x0]))
>             (label_ref 0)
>             (pc))) -1 (nil)
>     (expr_list:REG_BR_PROB (const_int 9900 [0x26ac])
>         (nil)))
> 
> 
> Notice the sign_extend in the PPC but not in the x86 case.

This RTL is indeed more elegant and probably less error-prone.
Comment 5 Eric Botcazou 2005-01-07 00:46:17 UTC 
Confirmed, it's the combiner optimistically getting rid of the HImode subreg.
Comment 6 Debian GCC Maintainers 2005-01-08 16:07:19 UTC 
[add reference to http://bugs.debian.org/288721]
Comment 7 Andrew Pinski 2005-01-09 02:12:53 UTC 
Patch here: <http://gcc.gnu.org/ml/gcc-patches/2005-01/msg00464.html>.
Comment 8 GCC Commits 2005-01-18 08:26:42 UTC 
Subject: Bug 19296

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	ebotcazou@gcc.gnu.org	2005-01-18 08:26:21

Modified files:
	gcc            : ChangeLog combine.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg: short-compare-1.c short-compare-2.c 

Log message:
	PR rtl-optimization/19296
	* combine.c (simplify_comparison): Rewrite the condition under
	which a non-paradoxical SUBREG of a PLUS can be lifted when
	compared against a constant.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.7168&r2=2.7169
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/combine.c.diff?cvsroot=gcc&r1=1.466&r2=1.467
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.4901&r2=1.4902
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/short-compare-1.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/short-compare-2.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
Comment 11 Eric Botcazou 2005-01-18 08:41:44 UTC 
Patch applied.