Message ID | mcrhbgswhuk.fsf@google.com |
---|---|
State | New |
Headers | show |
On Mon, Oct 11, 2010 at 2:16 PM, Ian Lance Taylor <iant@google.com> wrote: > The -fnon-call-exceptions options indicates that certain operations, > including division by zero, should generate an exception. This includes > cases like 0 / 0. This patch ensures that 0 / 0 and any other division > by zero is not eliminated when -fnon-call-exceptions is used. This > includes a test case which fails without the patch and passes with the > patch. > > This patch is entirely in the middle-end so I don't require approval. > However, I would like to see if there are any comments before I commit > this. Thanks. The testcase will never work on powerpc or spu or any other target where dividing by zero does not cause an exception to happen. (MIPS does a specific check for 0 and causes a trap). Not to mention I think on those targets we should remove the divide by zero still as they did not cause an exception to happen. Thanks, Andrew Pinski
Andrew Pinski <pinskia@gmail.com> writes: > On Mon, Oct 11, 2010 at 2:16 PM, Ian Lance Taylor <iant@google.com> wrote: >> The -fnon-call-exceptions options indicates that certain operations, >> including division by zero, should generate an exception. This includes >> cases like 0 / 0. This patch ensures that 0 / 0 and any other division >> by zero is not eliminated when -fnon-call-exceptions is used. This >> includes a test case which fails without the patch and passes with the >> patch. >> >> This patch is entirely in the middle-end so I don't require approval. >> However, I would like to see if there are any comments before I commit >> this. Thanks. > > The testcase will never work on powerpc or spu or any other target > where dividing by zero does not cause an exception to happen. (MIPS > does a specific check for 0 and causes a trap). Thanks. I changed the test to the appended. Are there other targets which should be excluded? > Not to mention I think on those targets we should remove the divide by > zero still as they did not cause an exception to happen. I don't think it's worth worrying about that minor optimization for an unusual case. Remember that this is only when using -fnon-call-exceptions. Ian
Ian Lance Taylor <iant@google.com> writes: > #ifdef SIGNAL_SUPPRESS > # define DO_TEST 0 > #elsif defined (__powerpc__) || defined (__PPC__) || defined (__ppc__) || defined (__POWERPC__) || defined (__ppc) s/elsif/elif/ C isn't Perl :-) Andreas.
Andreas Schwab <schwab@linux-m68k.org> writes: > Ian Lance Taylor <iant@google.com> writes: > >> #ifdef SIGNAL_SUPPRESS >> # define DO_TEST 0 >> #elsif defined (__powerpc__) || defined (__PPC__) || defined (__ppc__) || defined (__POWERPC__) || defined (__ppc) > > s/elsif/elif/ > > C isn't Perl :-) Thanks. Interesting problem--we don't warn about invalid preprocessing directives in code which is #ifdef'ed out. Ian
On Mon, Oct 11, 2010 at 02:44:02PM -0700, Ian Lance Taylor wrote: > #elsif defined (__powerpc__) || defined (__PPC__) || defined (__ppc__) || defined (__POWERPC__) || defined (__ppc) > /* On PPC division by zero does not trap. */ > # define DO_TEST 0 > #elsif defined (__SPU__) > /* On PPC division by zero does not trap. */ ^^^ Typo. -Nathan
Nathan Froyd <froydnj@codesourcery.com> writes: > On Mon, Oct 11, 2010 at 02:44:02PM -0700, Ian Lance Taylor wrote: >> #elsif defined (__powerpc__) || defined (__PPC__) || defined (__ppc__) || defined (__POWERPC__) || defined (__ppc) >> /* On PPC division by zero does not trap. */ >> # define DO_TEST 0 >> #elsif defined (__SPU__) >> /* On PPC division by zero does not trap. */ > ^^^ > > Typo. Sigh, thanks. Fixed in my copy. Ian
> The -fnon-call-exceptions options indicates that certain operations, > including division by zero, should generate an exception. This includes > cases like 0 / 0. This patch ensures that 0 / 0 and any other division > by zero is not eliminated when -fnon-call-exceptions is used. You must test cfun->can_throw_non_call_exceptions instead on the mainline.
On Tue, Oct 12, 2010 at 10:00 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> The -fnon-call-exceptions options indicates that certain operations, >> including division by zero, should generate an exception. This includes >> cases like 0 / 0. This patch ensures that 0 / 0 and any other division >> by zero is not eliminated when -fnon-call-exceptions is used. > > You must test cfun->can_throw_non_call_exceptions instead on the mainline. Indeed. Also I think your test is only trying to avoid optimization that happens downstream and is not a proper test for what it should test. It seems to be a proper test would be + if ((code == TRUNC_DIV_EXPR + || code == FLOOR_DIV_EXPR + || code == CEIL_DIV_EXPR + || code == EXACT_DIV_EXPR + || code == ROUND_DIV_EXPR) && cfun->can_throw_non_call_exceptions && (vr1.type != VR_RANGE || symbolic_range_p (&vr1) || range_includes_zero_p (&vr1))) which conservatively tests whether the range does include zero. If you only want to constrain the downstream optimization please instead integrate the tests into their respective predicates. Thanks, Richard. > -- > Eric Botcazou >
Richard Guenther <richard.guenther@gmail.com> writes: > On Tue, Oct 12, 2010 at 10:00 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >>> The -fnon-call-exceptions options indicates that certain operations, >>> including division by zero, should generate an exception. This includes >>> cases like 0 / 0. This patch ensures that 0 / 0 and any other division >>> by zero is not eliminated when -fnon-call-exceptions is used. >> >> You must test cfun->can_throw_non_call_exceptions instead on the mainline. > > Indeed. Also I think your test is only trying to avoid optimization that > happens downstream and is not a proper test for what it should test. > It seems to be a proper test would be > > + if ((code == TRUNC_DIV_EXPR > + || code == FLOOR_DIV_EXPR > + || code == CEIL_DIV_EXPR > + || code == EXACT_DIV_EXPR > + || code == ROUND_DIV_EXPR) > && cfun->can_throw_non_call_exceptions > && (vr1.type != VR_RANGE > || symbolic_range_p (&vr1) > || range_includes_zero_p (&vr1))) > > which conservatively tests whether the range does include zero. Argh, quite right. Thanks. Both corrections made. Ian
Index: tree-vrp.c =================================================================== --- tree-vrp.c (revision 165319) +++ tree-vrp.c (working copy) @@ -2464,6 +2464,22 @@ extract_range_from_binary_expr (value_ra } } + /* For divisions, if flag_non_call_exceptions is true, we must + not eliminate a division by zero. */ + if ((code == TRUNC_DIV_EXPR + || code == FLOOR_DIV_EXPR + || code == CEIL_DIV_EXPR + || code == EXACT_DIV_EXPR + || code == ROUND_DIV_EXPR) + && flag_non_call_exceptions + && vr1.type == VR_RANGE + && !symbolic_range_p (&vr1) + && range_includes_zero_p (&vr1)) + { + set_value_range_to_varying (vr); + return; + } + /* For divisions, if op0 is VR_RANGE, we can deduce a range even if op1 is VR_VARYING, VR_ANTI_RANGE, symbolic or can include 0. */ Index: simplify-rtx.c =================================================================== --- simplify-rtx.c (revision 165319) +++ simplify-rtx.c (working copy) @@ -2751,7 +2751,7 @@ simplify_binary_operation_1 (enum rtx_co else { /* 0/x is 0 (or x&0 if x has side-effects). */ - if (trueop0 == CONST0_RTX (mode)) + if (trueop0 == CONST0_RTX (mode) && !flag_non_call_exceptions) { if (side_effects_p (op1)) return simplify_gen_binary (AND, mode, op1, trueop0); Index: testsuite/gcc.c-torture/execute/20101011-1.c =================================================================== --- testsuite/gcc.c-torture/execute/20101011-1.c (revision 0) +++ testsuite/gcc.c-torture/execute/20101011-1.c (revision 0) @@ -0,0 +1,33 @@ +/* With -fnon-call-exceptions 0 / 0 should not be eliminated. The .x + file sets the option. */ + +#ifndef SIGNAL_SUPPRESS + +#include <signal.h> + +void +sigfpe (int signum __attribute__ ((unused))) +{ + exit (0); +} + +#endif /* !defined (SIGNAL_SUPPRESS) */ + +/* When optimizing, the compiler is smart enough to constant fold the + static unset variables i and j to produce 0 / 0, but it can't + eliminate the assignment to the global k. */ +static int i; +static int j; +int k; + +int +main () +{ +#ifndef SIGNAL_SUPPRESS + signal (SIGFPE, sigfpe); + k = i / j; + abort (); +#else /* defined (SIGNAL_SUPPRESS) */ + exit (0); +#endif /* defined (SIGNAL_SUPPRESS) */ +} Index: testsuite/gcc.c-torture/execute/20101011-1.x =================================================================== --- testsuite/gcc.c-torture/execute/20101011-1.x (revision 0) +++ testsuite/gcc.c-torture/execute/20101011-1.x (revision 0) @@ -0,0 +1,2 @@ +set additional_flags "-fnon-call-exceptions" +return 0