Bug 23109 - [4.1 Regression] compiler generates wrong code leading to spurious division by zero with -funsafe-math-optimizations (instead of -ftrapping-math)
Summary: [4.1 Regression] compiler generates wrong code leading to spurious division b...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.1.0
: P1 critical
Target Milestone: 4.1.0
Assignee: Paolo Bonzini
URL: http://gcc.gnu.org/ml/gcc-patches/200...
Keywords: patch, wrong-code
Depends on: 23948
Blocks:
  Show dependency treegraph
 
Reported: 2005-07-28 10:41 UTC by p.van-hoof
Modified: 2006-01-11 14:30 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.1.0
Last reconfirmed: 2005-07-28 15:01:58


Attachments
code that causes the crash (289 bytes, text/plain)
2005-07-28 10:42 UTC, p.van-hoof
Details

Note You need to log in before you can comment on or make changes to this bug.
Description p.van-hoof 2005-07-28 10:41:54 UTC
The attached code crashes on a division by zero when compiled as follows:

dogbert> gcc -O1 -funsafe-math-optimizations -ftrapping-math b.c
dogbert> a.out
Floating exception
dogbert> gcc -v
Using built-in specs.
Target: i686-pc-linux-gnu
Configured with: ../gcc/configure --prefix=/usr/local/gcc410
--enable-languages=c,c++,f95
Thread model: posix
gcc version 4.1.0 20050725 (experimental)

I don't believe that -funsafe-math-optimizations enables -fno-trapping-math, but
to be really sure I added the -ftrapping-math command line option. It makes no
difference.

Taken at face value it looks like the optimizer lifts the division by d outside
the conditional on line 19, even though that could lead to a spurious division
by zero and -ftrapping-math is in effect.

This behavior only shows up on the mainline, and not on the 3.4 and 4.0
branches. A slightly modified version (to enable FP traps on Suns) also crashes
with gcc 4.1.0 on the target sparc-sun-solaris2.9.
Comment 1 p.van-hoof 2005-07-28 10:42:52 UTC
Created attachment 9377 [details]
code that causes the crash
Comment 2 Richard Biener 2005-07-28 14:47:37 UTC
Indeed unsafe-math-optimization is not the only precondition for this
transformation.
Comment 3 Richard Biener 2005-07-28 15:01:58 UTC
I have a patch.
Comment 4 GCC Commits 2005-08-01 08:53:10 UTC
Subject: Bug 23109

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	rguenth@gcc.gnu.org	2005-08-01 08:53:00

Modified files:
	gcc            : ChangeLog tree-ssa-loop-im.c 
	                 tree-ssa-math-opts.c 

Log message:
	2005-08-01  Richard Guenther  <rguenther@suse.de>
	
	PR tree-optimization/23109
	* tree-ssa-math-opts.c (execute_cse_reciprocals_1):
	If trapping math is in effect, use post-dominator information
	to check if we'd in any case reach a trapping point before
	doing the reciprocal insertion.
	(execute_cse_reciprocals): Compute post-dominators, if necessary.
	* tree-ssa-loop-im.c (determine_invariantness_stmt): RDIV
	expressions are invariant only if trapping math is not in effect.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.9622&r2=2.9623
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/tree-ssa-loop-im.c.diff?cvsroot=gcc&r1=2.48&r2=2.49
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/tree-ssa-math-opts.c.diff?cvsroot=gcc&r1=2.2&r2=2.3

Comment 5 Richard Biener 2005-08-01 08:54:43 UTC
Fixed.
Comment 6 Paolo Bonzini 2005-10-14 12:00:13 UTC
I'm not sure this is really fixed.

With this test case,

float a = g();
printf ("%g\n", a);
printf ("%g\n", 2 / a);
printf ("%g\n", 3 / a);

the division is inserted before the *first* printf, while I believe it should be inserted before the second.

The patch in pr23948 tries to insert reciprocals before the use point, rather than after the def point, which would fix this as well.

Should this bug be reopened?
Comment 7 Richard Biener 2005-10-14 12:13:48 UTC
Yes, I think so.  You don't even need printf, but any observable side-effect,
like a store to a global will do.
Comment 8 Mark Mitchell 2005-10-31 04:20:43 UTC
This is a serious wrong-code problem; it's a showstopper.
Comment 9 Paolo Bonzini 2005-11-01 22:00:13 UTC
It is a showstopper, but a patch has been ready for months...  I admit the patch is non-trivial, but the testcases are comprehensive.

Maybe it's best to wait until the branch and backport it a week later.  But I don't think so.
Comment 10 Richard Biener 2005-11-01 22:17:33 UTC
Note that the patch is in the suse compiler since three weeks now without a problem.
Comment 11 Paolo Bonzini 2005-11-03 10:05:05 UTC
Note that while PR23948 could be fixed by hacking the current recip pass so that it inserts the division into another basic block, this is not true of this bug. 
Comment 12 Steven Bosscher 2005-11-16 14:12:06 UTC
Is this bug going anywhere???
Comment 13 Richard Biener 2005-11-16 14:16:51 UTC
Nobody reviewed the patch AFAIK.  Still the patch hasn't caused any problems sofar in the SUSE compiler.
Comment 14 Paolo Bonzini 2005-11-16 14:46:23 UTC
I have little hope of seeing this reviewed before the branch.  But it should be put in 4.2 very early so that we backport it well before 4.1.0 is released.

BTW I think that 23948 and 24123 are way more severe than 23109 even though this one is a wrong code bug.  These three bugs are three almost-showstoppers, and are actually different manifestations of the same basic flaw (recip should insert temporaries near the uses, not near the defs).  PR23948 makes -ffast-math unusable in C++, and PR24123 generates really abysmal code (20x slower!).

Paolo
Comment 15 Paolo Bonzini 2006-01-11 13:02:24 UTC
Subject: Bug 23109

Author: bonzini
Date: Wed Jan 11 13:02:18 2006
New Revision: 109578

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=109578
Log:
gcc:
2006-01-11  Paolo Bonzini  <bonzini@gnu.org>

	PR tree-optimization/23109
	PR tree-optimization/23948
	PR tree-optimization/24123

	* Makefile.in (tree-ssa-math-opts.o): Adjust dependencies.
        * tree-cfg.c (single_noncomplex_succ): New.
        * tree-flow.h (single_noncomplex_succ): Declare it.
        * tree-ssa-math-opts.c (enum place_reciprocal): Remove.
        * tree-ssa-math-opts.c (enum place_reciprocal): Remove.
        (struct occurrence, occ_head, occ_pool, is_divide_by, compute_merit,
	insert_bb, register_division_in, insert_reciprocals,
	replace_reciprocal, free_bb): New.
        (execute_cse_reciprocals_1): Rewritten.
        (execute_cse_reciprocals): Adjust calls to execute_cse_reciprocals_1.
        Do not commit any edge insertion.  Always compute dominators and
        create the allocation pool.
        * target-def.h (TARGET_MIN_DIVISIONS_FOR_RECIP_MUL): New.
	* target.h (struct gcc_target): Add min_divistions_for_recip_mul.
	* targhooks.c (default_min_divistions_for_recip_mul): New.
	* targhooks.h (default_min_divistions_for_recip_mul): New prototype.
        * passes.c (init_optimization_passes): Run recip after tree loop
        optimizations.
        * doc/tm.texi (Misc): Document TARGET_MIN_DIVISIONS_FOR_RECIP_MUL.

gcc/testsuite:
2006-01-11  Paolo Bonzini  <bonzini@gnu.org>
        
        PR tree-optimization/23109
        PR tree-optimization/23948
        PR tree-optimization/24123

        * gcc.dg/tree-ssa/recip-3.c, gcc.dg/tree-ssa/recip-4.c,
        gcc.dg/tree-ssa/recip-5.c, gcc.dg/tree-ssa/recip-6.c,
        gcc.dg/tree-ssa/recip-7.c, gcc.dg/tree-ssa/pr23109.c,
        g++.dg/tree-ssa/pr23948.C: New testcases.
        * gcc.dg/tree-ssa/recip-2.c, gcc.dg/tree-ssa/pr23234.c: Provide
	three divisions in order to do the optimization.


Added:
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr23948.C
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr23109.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-3.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-4.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-5.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-6.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-7.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/Makefile.in
    trunk/gcc/doc/tm.texi
    trunk/gcc/passes.c
    trunk/gcc/target-def.h
    trunk/gcc/target.h
    trunk/gcc/targhooks.c
    trunk/gcc/targhooks.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr23234.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-2.c
    trunk/gcc/tree-cfg.c
    trunk/gcc/tree-flow.h
    trunk/gcc/tree-ssa-math-opts.c

Comment 16 Paolo Bonzini 2006-01-11 14:29:38 UTC
Subject: Bug 23109

Author: bonzini
Date: Wed Jan 11 14:29:29 2006
New Revision: 109586

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=109586
Log:
gcc:
2006-01-11  Paolo Bonzini  <bonzini@gnu.org>

	PR tree-optimization/23109
	PR tree-optimization/23948
	PR tree-optimization/24123

	* Makefile.in (tree-ssa-math-opts.o): Adjust dependencies.
        * tree-cfg.c (single_noncomplex_succ): New.
        * tree-flow.h (single_noncomplex_succ): Declare it.
        * tree-ssa-math-opts.c (enum place_reciprocal): Remove.
        * tree-ssa-math-opts.c (enum place_reciprocal): Remove.
        (struct occurrence, occ_head, occ_pool, is_divide_by, compute_merit,
	insert_bb, register_division_in, insert_reciprocals,
	replace_reciprocal, free_bb): New.
        (execute_cse_reciprocals_1): Rewritten.
        (execute_cse_reciprocals): Adjust calls to execute_cse_reciprocals_1.
        Do not commit any edge insertion.  Always compute dominators and
        create the allocation pool.
        * target-def.h (TARGET_MIN_DIVISIONS_FOR_RECIP_MUL): New.
	* target.h (struct gcc_target): Add min_divistions_for_recip_mul.
	* targhooks.c (default_min_divistions_for_recip_mul): New.
	* targhooks.h (default_min_divistions_for_recip_mul): New prototype.
        * passes.c (init_optimization_passes): Run recip after tree loop
        optimizations.
        * doc/tm.texi (Misc): Document TARGET_MIN_DIVISIONS_FOR_RECIP_MUL.

gcc/testsuite:
2006-01-11  Paolo Bonzini  <bonzini@gnu.org>
        
        PR tree-optimization/23109
        PR tree-optimization/23948
        PR tree-optimization/24123

        * gcc.dg/tree-ssa/recip-3.c, gcc.dg/tree-ssa/recip-4.c,
        gcc.dg/tree-ssa/recip-5.c, gcc.dg/tree-ssa/recip-6.c,
        gcc.dg/tree-ssa/recip-7.c, gcc.dg/tree-ssa/pr23109.c,
        g++.dg/tree-ssa/pr23948.C: New testcases.
        * gcc.dg/tree-ssa/recip-2.c, gcc.dg/tree-ssa/pr23234.c: Provide
	three divisions in order to do the optimization.


Added:
    branches/gcc-4_1-branch/gcc/testsuite/g++.dg/tree-ssa/pr23948.C
      - copied unchanged from r109578, trunk/gcc/testsuite/g++.dg/tree-ssa/pr23948.C
    branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/tree-ssa/pr23109.c
      - copied unchanged from r109578, trunk/gcc/testsuite/gcc.dg/tree-ssa/pr23109.c
    branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/tree-ssa/recip-3.c
      - copied unchanged from r109578, trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-3.c
    branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/tree-ssa/recip-4.c
      - copied unchanged from r109578, trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-4.c
    branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/tree-ssa/recip-5.c
      - copied unchanged from r109578, trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-5.c
    branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/tree-ssa/recip-6.c
      - copied unchanged from r109578, trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-6.c
    branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/tree-ssa/recip-7.c
      - copied unchanged from r109578, trunk/gcc/testsuite/gcc.dg/tree-ssa/recip-7.c
Modified:
    branches/gcc-4_1-branch/gcc/ChangeLog
    branches/gcc-4_1-branch/gcc/Makefile.in
    branches/gcc-4_1-branch/gcc/doc/tm.texi
    branches/gcc-4_1-branch/gcc/passes.c
    branches/gcc-4_1-branch/gcc/target-def.h
    branches/gcc-4_1-branch/gcc/target.h
    branches/gcc-4_1-branch/gcc/targhooks.c
    branches/gcc-4_1-branch/gcc/targhooks.h
    branches/gcc-4_1-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/tree-ssa/pr23234.c
    branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/tree-ssa/recip-2.c
    branches/gcc-4_1-branch/gcc/tree-cfg.c
    branches/gcc-4_1-branch/gcc/tree-flow.h
    branches/gcc-4_1-branch/gcc/tree-ssa-math-opts.c

Comment 17 Paolo Bonzini 2006-01-11 14:30:18 UTC
patch committed to 4.1 too