diff mbox

PATCH RFC: With -fnon-call-exceptions don't eliminate division by zero

Message ID mcrhbgswhuk.fsf@google.com
State New
Headers show

Commit Message

Ian Lance Taylor Oct. 11, 2010, 9:16 p.m. UTC 
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.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

Ian


gcc/ChangeLog:

2010-10-11  Ian Lance Taylor  <iant@google.com>

	* tree-vrp.c (extract_range_from_binary_expr): If
	flag_non_call_exceptions don't eliminate division by zero.
	* simplify-rtx.c (simplify_binary_operation_1): Likewise.

gcc/testsuite/ChangeLog:

2010-10-11  Ian Lance Taylor  <iant@google.com>

	* gcc.c-torture/execute/20101011-1.c: New test.
	* gcc.c-torture/execute/20101011-1.x: New test driver.

Comments

Andrew Pinski Oct. 11, 2010, 9:21 p.m. UTC | #1 
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
Ian Lance Taylor Oct. 11, 2010, 9:44 p.m. UTC | #2 
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
Andreas Schwab Oct. 11, 2010, 10:29 p.m. UTC | #3 
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.
Ian Lance Taylor Oct. 11, 2010, 10:56 p.m. UTC | #4 
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
Nathan Froyd Oct. 11, 2010, 10:59 p.m. UTC | #5 
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
Ian Lance Taylor Oct. 11, 2010, 11:06 p.m. UTC | #6 
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
Eric Botcazou Oct. 12, 2010, 8 a.m. UTC | #7 
> 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.
Richard Biener Oct. 12, 2010, 10:22 a.m. UTC | #8 
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
>
Ian Lance Taylor Oct. 12, 2010, 1:48 p.m. UTC | #9 
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
diff mbox

Patch

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