Bug 29519 - [4.2/4.3 Regression] Bad code on MIPS with -fnon-call-exceptions
Summary: [4.2/4.3 Regression] Bad code on MIPS with -fnon-call-exceptions
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.2.0
: P3 normal
Target Milestone: 4.2.0
Assignee: David Daney
URL:
Keywords: patch, wrong-code
Depends on:
Blocks:
 
Reported: 2006-10-19 21:59 UTC by David Daney
Modified: 2006-10-25 22:34 UTC (History)
4 users (show)

See Also:
Host: i686-pc-linux-gnu
Target: mipsel-linux-gnu
Build: i686-pc-linux-gnu
Known to work: 3.4.3
Known to fail: 4.2.0
Last reconfirmed: 2006-10-23 15:00:07


Attachments
Dumps from -da with and without -fnon-call-exceptions (6.52 KB, application/x-gzip)
2006-10-19 22:54 UTC, David Daney
Details
Potential fix. (398 bytes, patch)
2006-10-22 04:11 UTC, David Daney
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Daney 2006-10-19 21:59:40 UTC
This is a reduced testcase from a failure I am seeing on libgcj testsuite.  java.lang.Class.isPrimitive() is being compiled incorrectly.

This is a regression from 3.4.3.

Configured as: ../clean/configure   --target=mipsel-linux --with-sysroot=/usr/local/mipsel-linux-test --prefix=/usr/local/mipsel-linux-test --with-arch=mips32 --with-float=soft --with-system-zlib --disable-java-awt --without-x --disable-libmudflap --disable-libgomp --disable-jvmpi --disable-tls --enable-languages=c,c++,java

on i686-pc-linux-gnu

Here is the test case in C  isprim.c:
-------------8<------------
struct F;
struct C
{
    struct F *f13;
};
int isM1(struct C *t)
{
    return t->f13 == (struct F*)-1;
}
-------------8<------------
$ mipsel-linux-gcc -O1 -S isprim.c -o isprim.s

Generates this correct code:
	lw	$2,0($4)
	addiu	$2,$2,1
	j	$31
	sltu	$2,$2,1


$ mipsel-linux-gcc -fnon-call-exceptions -O1 -S isprim.c -o isprim.s

Generates this incorrect code that unconditionally returns 0:

	lw	$2,0($4)
	.set	noreorder
	.set	nomacro
	j	$31
	move	$2,$0
	.set	macro
	.set	reorder
Comment 1 David Daney 2006-10-19 22:54:29 UTC
Created attachment 12465 [details]
Dumps from -da with and without -fnon-call-exceptions

The dump files in da-dumps.tgz named isprimb.c.* are from:
$ mipsel-linux-gcc -fnon-call-exceptions -da -O1 -S isprimb.c -o isprimb.s
Where isprimb.c is the exact same testcase as isprim.c  The 'b' suffix indicating bad.

The files named isprim.c.* are from:
$ mipsel-linux-gcc -da -O1 -S isprim.c -o isprim.s
Comment 2 Andrew Pinski 2006-10-19 23:10:44 UTC
Expand does:

;; return t->f13 == 4294967295B
(insn 10 9 11 (set (reg/f:SI 196)
        (mem/s/f/j:SI (reg/v/f:SI 194 [ t ]) [0 <variable>.f13+0 S4 A32])) -1 (nil)
    (nil))

(insn 11 10 12 (set (reg:SI 198)
        (plus:SI (reg/f:SI 196)
            (const_int 1 [0x1]))) -1 (nil)
    (nil))

(insn 12 11 13 (set (reg:SI 197)
        (eq:SI (reg:SI 198)
            (const_int 0 [0x0]))) -1 (nil)
    (nil))

(insn 13 12 14 (set (reg:SI 193 [ <result> ])
        (reg:SI 197)) -1 (nil)
    (nil))

(jump_insn 14 13 15 (set (pc)
        (label_ref 0)) -1 (nil)
    (nil))


And then CSE1 changes that into:
(insn 6 8 7 2 (set (reg/v/f:SI 194 [ t ])
        (reg:SI 4 $4 [ t ])) 213 {*movsi_internal} (nil)
    (expr_list:REG_EQUIV (mem/f/c/i:SI (reg/f:SI 77 $arg) [0 t+0 S4 A32])
        (nil)))

(note 7 6 10 2 NOTE_INSN_FUNCTION_BEG)

(insn 10 7 16 2 (set (reg/f:SI 196 [ <variable>.f13 ])
        (mem/s/f/j:SI (reg/v/f:SI 194 [ t ]) [0 <variable>.f13+0 S4 A32])) 213 {*movsi_internal} (nil)
    (nil))

(note 16 10 19 2 NOTE_INSN_FUNCTION_END)

(insn 19 16 20 2 (asm_input ("")) -1 (nil)
    (nil))

(insn 20 19 26 2 (set (reg/i:SI 2 $2 [ <result> ])
        (const_int 0 [0x0])) 213 {*movsi_internal} (nil)
    (nil))
Comment 3 Andrew Pinski 2006-10-19 23:16:26 UTC
The difference between with and without -fnon-call-exceptions is:
insn 10 9 11 3 (set (reg/f:SI 196)
        (mem/s/f/j:SI (reg/v/f:SI 194 [ t ]) [0 <variable>.f13+0 S4 A32])) -1 (nil)
    (nil))

vs:

(insn 10 9 11 3 (set (reg:SI 196)
        (mem/s/f/j:SI (reg/v/f:SI 194 [ t ]) [0 <variable>.f13+0 S4 A32])) -1 (nil)
    (nil))


Note in the case where it is set, we record 196 as a pointer so we know that pointers don't wrap so we decide that pointer+1 can never be 0 in CSE.
Comment 4 David Daney 2006-10-20 01:10:28 UTC
Caused by commit r117143

Thanks to Andrew Pinski for helping me find it.
Comment 5 rsandifo@gcc.gnu.org 2006-10-21 18:44:16 UTC
The wrapping check was added by:

    http://gcc.gnu.org/ml/gcc-patches/2002-10/msg00360.html

I think this check is wrong for exactly the reason seen here;
without type information, and without context, there's no
guarantee that the PLUS itself is a pointer, or indeed that
the constant was originally positive.  We could only do
something like this if we know the address is being
dereferenced -- but in that case, we generally care about
whether the value might trap, not whether it's zero
(and rtlanal.c rightly says that (plus reg (const_int ...))
can trap).

For example, something like:

    (uintptr_t) foo == 0x90000000

could legitimately be implemented as:

    (eq (plus foo (const_int 0x70000000)) (const_int 0))

And AIUI, gcc allows things like:

    (uintptr_t) foo - 0x90000000

and allows the result to be zero (assuming the result is not
dereferenced).

All uses of nonzero_address_p appear to using it for
general rtx simplification, i.e. in cases where the value
is not known to be a pointer.  So I think this check should
just be removed.

Hopefully the check is less important now than it was when
it was originally committed.  This sort of optimisation can
be done at the tree level instead.

Richard
Comment 6 David Daney 2006-10-22 04:11:04 UTC
Created attachment 12474 [details]
Potential fix.

As per Richard's analysis, I am testing this patch.  It seems to fix the testcase.  I will bootstrap and test i686-pc-linux-gnu as well as mipsel-linux-gnu.

Richard, does this look right?
Comment 7 richard@codesourcery.com 2006-10-22 08:51:12 UTC
Subject: Re:  [4.2 Regression] Bad code on MIPS with -fnon-call-exceptions

"daney at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes:
> Richard, does this look right?

Looks good to me.
Comment 8 David Daney 2006-10-23 15:00:07 UTC
The patch is here:
http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01149.html
Comment 9 David Daney 2006-10-25 05:49:53 UTC
Subject: Bug 29519

Author: daney
Date: Wed Oct 25 05:49:43 2006
New Revision: 118023

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=118023
Log:
	PR middle-end/29519
	* rtlanal.c (nonzero_address_p):  Remove check for values wrapping.

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

Comment 10 David Daney 2006-10-25 22:29:27 UTC
Subject: Bug 29519

Author: daney
Date: Wed Oct 25 22:29:10 2006
New Revision: 118044

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=118044
Log:
	PR middle-end/29519
	* rtlanal.c (nonzero_address_p):  Remove check for values wrapping.

Modified:
    branches/gcc-4_2-branch/gcc/ChangeLog
    branches/gcc-4_2-branch/gcc/rtlanal.c

Comment 11 David Daney 2006-10-25 22:34:09 UTC
With the (now committed) patches, my testing shows the problem as fixed.