diff mbox

Fix 61441

Message ID CA+ZXfhWh9qmyNACyYDzakHv1qvd-JLWZGr2eeg--KL4OyU15xg@mail.gmail.com
State New
Headers show

Commit Message

Sujoy Saraswati Oct. 13, 2015, 10:46 a.m. UTC 
Hi,
 This is another modified version of the patch, incorporating the
previous comments.

Bootstrap and regression tests on x86_64-linux-gnu and
aarch64-unknown-linux-gnu passed with changes done on trunk.

Is this fine ?

Regards,
Sujoy

2015-10-13  Sujoy Saraswati <ssaraswati@gmail.com>

        PR tree-optimization/61441
        * builtins.c (integer_valued_real_p): Return true for
        NaN values.
        (fold_builtin_trunc, fold_builtin_pow): Avoid the operation
        if flag_signaling_nans is on and the operand is a NaN.
        (fold_builtin_powi): Same.
        * fold-const.c (const_binop): Convert sNaN to qNaN when
        flag_signaling_nans is off.
        (const_unop): Avoid the operation, other than NEGATE and
        ABS, if flag_signaling_nans is on and the operand is a NaN.
        (fold_convert_const_real_from_real): Avoid the operation if
        flag_signaling_nans is on and the operand is a NaN.
        * real.c (do_add): Make resulting NaN value to be qNaN.
        (do_multiply, do_divide, do_fix_trunc): Same.
        (real_arithmetic, real_ldexp): Same
        * simplify-rtx.c (simplify_const_unary_operation): Avoid the
        operation if flag_signaling_nans is on and the operand is a NaN.
        * tree-ssa-math-opts.c (gimple_expand_builtin_pow): Same.

        PR tree-optimization/61441
        * gcc.dg/pr61441.c: New testcase.


On Wed, Sep 16, 2015 at 10:09 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Wed, 16 Sep 2015, Sujoy Saraswati wrote:
>
>> > If -fsignaling-nans, then folding of expressions involving sNaNs should be
>> > disabled, outside of static initializers - such expressions should not get
>> > folded to return an sNaN (it's incorrect to fold sNaN + 1 to sNaN, for
>> > example).  I think existing code may ensure that (the HONOR_SNANS check in
>> > const_binop, for example).
>>
>> Yes, with -fsignaling-nans, the const_binop will not fold since the
>> HONOR_SNANS check is there. However, elsewhere, like const_unop, the
>> code doesn't do this check.
>
> Which would be a bug in the const_unop code, or the functions it calls
> (for operations for which such a check is appropriate - as noted, abs and
> negation should be folded unconditionally).
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Comments

Joseph Myers Oct. 28, 2015, 5:04 p.m. UTC | #1 
On Tue, 13 Oct 2015, Sujoy Saraswati wrote:

> Index: gcc/builtins.c
> ===================================================================
> --- gcc/builtins.c      (revision 228700)
> +++ gcc/builtins.c      (working copy)
> @@ -7357,7 +7357,11 @@ integer_valued_real_p (tree t)
>              && integer_valued_real_p (TREE_OPERAND (t, 2));
> 
>      case REAL_CST:
> -      return real_isinteger (TREE_REAL_CST_PTR (t), TYPE_MODE (TREE_TYPE (t)));
> +      /* Return true for NaN values, since real_isinteger would
> +         return false if the value is sNaN.  */
> +      return (REAL_VALUE_ISNAN (TREE_REAL_CST (t))
> +              || real_isinteger (TREE_REAL_CST_PTR (t),
> +                             TYPE_MODE (TREE_TYPE (t))));

It seems this code has moved to fold-const.c, so the patch will need 
updating.

Now, is it actually correct for integer_valued_real_single_p (now) to 
treat sNaN as an integer?  Looking at existing uses, they all appear to be 
in match.pd: it's used for removing redundant trunc / floor / ceil / round 
/ nearbyint on integer values.  In such cases, it's *incorrect* to count 
sNaN as an integer, because while those functions map qNaN quietly to 
qNaN, they should map sNaN to qNaN and raise "invalid" in the process.  So 
I don't think you should have this change at all.

(I don't see why the handling of rint checks flag_errno_math.  rint should 
quietly return infinite and qNaN arguments just like nearbyint; it never 
needs to set errno and distinguishing rint / nearbyint here is spurious; 
the only difference is regarding the "inexact" exception.)

> @@ -7910,8 +7914,13 @@ fold_builtin_trunc (location_t loc, tree fndecl, t
>        tree type = TREE_TYPE (TREE_TYPE (fndecl));
> 
>        x = TREE_REAL_CST (arg);
> -      real_trunc (&r, TYPE_MODE (type), &x);
> -      return build_real (type, r);
> +      /* Avoid the folding if flag_signaling_nans is on.  */
> +      if (!(HONOR_SNANS (TYPE_MODE (type))
> +            && REAL_VALUE_ISNAN (x)))

I realise this corresponds to some existing code (in const_binop, at 
least), but I don't think that existing code is a good model.

If -fsignaling-nans, it's still perfectly fine to fold when the argument 
is a quiet NaN.  So what you actually want in such cases where you have a 
known constant argument - as opposed to an unknown argument that might or 
might not be constant - is to check whether that argument is itself a 
signaling NaN.  So you want to add a new REAL_VALUE_ISSIGNALING, or 
something like that.  And then find existing code that has the wrong (NaN 
and sNaNs supported) check and convert it to checking (this value is 
sNaN).

(This code has also moved to another function in fold-const.c, it seems.)

Cf. the match.pd handling of fmin / fmax checking for signaling NaN 
arguments explicitly.

Some other places in this patch have similar issues with checking 
HONOR_SNANS && REAL_VALUE_ISNAN when they should check if the particular 
value is sNaN.
diff mbox

Patch

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c      (revision 228700)
+++ gcc/builtins.c      (working copy)
@@ -7357,7 +7357,11 @@  integer_valued_real_p (tree t)
             && integer_valued_real_p (TREE_OPERAND (t, 2));

     case REAL_CST:
-      return real_isinteger (TREE_REAL_CST_PTR (t), TYPE_MODE (TREE_TYPE (t)));
+      /* Return true for NaN values, since real_isinteger would
+         return false if the value is sNaN.  */
+      return (REAL_VALUE_ISNAN (TREE_REAL_CST (t))
+              || real_isinteger (TREE_REAL_CST_PTR (t),
+                             TYPE_MODE (TREE_TYPE (t))));

     CASE_CONVERT:
       {
@@ -7910,8 +7914,13 @@  fold_builtin_trunc (location_t loc, tree fndecl, t
       tree type = TREE_TYPE (TREE_TYPE (fndecl));

       x = TREE_REAL_CST (arg);
-      real_trunc (&r, TYPE_MODE (type), &x);
-      return build_real (type, r);
+      /* Avoid the folding if flag_signaling_nans is on.  */
+      if (!(HONOR_SNANS (TYPE_MODE (type))
+            && REAL_VALUE_ISNAN (x)))
+      {
+        real_trunc (&r, TYPE_MODE (type), &x);
+        return build_real (type, r);
+      }
     }

   return fold_trunc_transparent_mathfn (loc, fndecl, arg);
@@ -8297,9 +8306,15 @@  fold_builtin_pow (location_t loc, tree fndecl, tre
              bool inexact;

              x = TREE_REAL_CST (arg0);
+
              inexact = real_powi (&x, TYPE_MODE (type), &x, n);
-             if (flag_unsafe_math_optimizations || !inexact)
-               return build_real (type, x);
+
+              /* Avoid the folding if flag_signaling_nans is on.  */
+             if (flag_unsafe_math_optimizations
+                  || (!inexact
+                      && !(HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg0)))
+                           && REAL_VALUE_ISNAN (x))))
+                 return build_real (type, x);
            }

          /* Strip sign ops from even integer powers.  */
@@ -8388,8 +8403,14 @@  fold_builtin_powi (location_t loc, tree fndecl ATT
        {
          REAL_VALUE_TYPE x;
          x = TREE_REAL_CST (arg0);
-         real_powi (&x, TYPE_MODE (type), &x, c);
-         return build_real (type, x);
+
+          /* Avoid the folding if flag_signaling_nans is on.  */
+          if (!(HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg0)))
+                && REAL_VALUE_ISNAN (x)))
+          {
+           real_powi (&x, TYPE_MODE (type), &x, c);
+           return build_real (type, x);
+          }
        }

       /* Optimize pow(x,0) = 1.0.  */
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c    (revision 228700)
+++ gcc/fold-const.c    (working copy)
@@ -1185,9 +1185,21 @@  const_binop (enum tree_code code, tree arg1, tree
       /* If either operand is a NaN, just return it.  Otherwise, set up
         for floating-point trap; we return an overflow.  */
       if (REAL_VALUE_ISNAN (d1))
-       return arg1;
+      {
+        /* Make resulting NaN value to be qNaN when flag_signaling_nans
+           is off.  */
+        d1.signalling = 0;
+        t = build_real (type, d1);
+       return t;
+      }
       else if (REAL_VALUE_ISNAN (d2))
-       return arg2;
+      {
+        /* Make resulting NaN value to be qNaN when flag_signaling_nans
+           is off.  */
+        d2.signalling = 0;
+        t = build_real (type, d2);
+       return t;
+      }

       inexact = real_arithmetic (&value, code, &d1, &d2);
       real_convert (&result, mode, &value);
@@ -1557,6 +1569,15 @@  const_binop (enum tree_code code, tree type, tree
 tree
 const_unop (enum tree_code code, tree type, tree arg0)
 {
+  /* Don't perform the operation, other than NEGATE and ABS, if
+     flag_signaling_nans is on and the operand is a NaN.  */
+  if (TREE_CODE (arg0) == REAL_CST
+      && HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg0)))
+      && REAL_VALUE_ISNAN (TREE_REAL_CST (arg0))
+      && code != NEGATE_EXPR
+      && code != ABS_EXPR)
+    return NULL_TREE;
+
   switch (code)
     {
     CASE_CONVERT:
@@ -1964,7 +1985,17 @@  fold_convert_const_real_from_real (tree type, cons
   REAL_VALUE_TYPE value;
   tree t;

+  /* Don't perform the operation if flag_signaling_nans is on
+     and the operand is a NaN.  */
+  if (HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg1)))
+      && REAL_VALUE_ISNAN (TREE_REAL_CST (arg1)))
+    return NULL_TREE;
+
   real_convert (&value, TYPE_MODE (type), &TREE_REAL_CST (arg1));
+  /* Make resulting NaN value to be qNaN when flag_signaling_nans
+     is off.  */
+  if (REAL_VALUE_ISNAN (value))
+    value.signalling = 0;
   t = build_real (type, value);

   /* If converting an infinity or NAN to a representation that doesn't
Index: gcc/real.c
===================================================================
--- gcc/real.c  (revision 228700)
+++ gcc/real.c  (working copy)
@@ -545,6 +545,10 @@  do_add (REAL_VALUE_TYPE *r, const REAL_VALUE_TYPE
     case CLASS2 (rvc_normal, rvc_inf):
       /* R + Inf = Inf.  */
       *r = *b;
+      /* Make resulting NaN value to be qNaN. The caller has the
+         responsibility to avoid the operation if flag_signaling_nans
+         is on.  */
+      r->signalling = 0;
       r->sign = sign ^ subtract_p;
       return false;

@@ -558,6 +562,10 @@  do_add (REAL_VALUE_TYPE *r, const REAL_VALUE_TYPE
     case CLASS2 (rvc_inf, rvc_normal):
       /* Inf + R = Inf.  */
       *r = *a;
+      /* Make resulting NaN value to be qNaN. The caller has the
+         responsibility to avoid the operation if flag_signaling_nans
+         is on.  */
+      r->signalling = 0;
       return false;

     case CLASS2 (rvc_inf, rvc_inf):
@@ -680,6 +688,10 @@  do_multiply (REAL_VALUE_TYPE *r, const REAL_VALUE_
     case CLASS2 (rvc_nan, rvc_nan):
       /* ANY * NaN = NaN.  */
       *r = *b;
+      /* Make resulting NaN value to be qNaN. The caller has the
+         responsibility to avoid the operation if flag_signaling_nans
+         is on.  */
+      r->signalling = 0;
       r->sign = sign;
       return false;

@@ -688,6 +700,10 @@  do_multiply (REAL_VALUE_TYPE *r, const REAL_VALUE_
     case CLASS2 (rvc_nan, rvc_inf):
       /* NaN * ANY = NaN.  */
       *r = *a;
+      /* Make resulting NaN value to be qNaN. The caller has the
+         responsibility to avoid the operation if flag_signaling_nans
+         is on.  */
+      r->signalling = 0;
       r->sign = sign;
       return false;

@@ -830,6 +846,10 @@  do_divide (REAL_VALUE_TYPE *r, const REAL_VALUE_TY
     case CLASS2 (rvc_nan, rvc_nan):
       /* ANY / NaN = NaN.  */
       *r = *b;
+      /* Make resulting NaN value to be qNaN. The caller has the
+         responsibility to avoid the operation if flag_signaling_nans
+         is on.  */
+      r->signalling = 0;
       r->sign = sign;
       return false;

@@ -838,6 +858,10 @@  do_divide (REAL_VALUE_TYPE *r, const REAL_VALUE_TY
     case CLASS2 (rvc_nan, rvc_inf):
       /* NaN / ANY = NaN.  */
       *r = *a;
+      /* Make resulting NaN value to be qNaN. The caller has the
+         responsibility to avoid the operation if flag_signaling_nans
+         is on.  */
+      r->signalling = 0;
       r->sign = sign;
       return false;

@@ -968,6 +992,10 @@  do_fix_trunc (REAL_VALUE_TYPE *r, const REAL_VALUE
     case rvc_zero:
     case rvc_inf:
     case rvc_nan:
+      /* Make resulting NaN value to be qNaN. The caller has the
+         responsibility to avoid the operation if flag_signaling_nans
+         is on.  */
+      r->signalling = 0;
       break;

     case rvc_normal:
@@ -1026,7 +1054,13 @@  real_arithmetic (REAL_VALUE_TYPE *r, int icode, co

     case MIN_EXPR:
       if (op1->cl == rvc_nan)
+      {
        *r = *op1;
+        /* Make resulting NaN value to be qNaN. The caller has the
+           responsibility to avoid the operation if flag_signaling_nans
+           is on.  */
+        r->signalling = 0;
+      }
       else if (do_compare (op0, op1, -1) < 0)
        *r = *op0;
       else
@@ -1035,7 +1069,13 @@  real_arithmetic (REAL_VALUE_TYPE *r, int icode, co

     case MAX_EXPR:
       if (op1->cl == rvc_nan)
+      {
        *r = *op1;
+        /* Make resulting NaN value to be qNaN. The caller has the
+           responsibility to avoid the operation if flag_signaling_nans
+           is on.  */
+        r->signalling = 0;
+      }
       else if (do_compare (op0, op1, 1) < 0)
        *r = *op1;
       else
@@ -1166,6 +1206,10 @@  real_ldexp (REAL_VALUE_TYPE *r, const REAL_VALUE_T
     case rvc_zero:
     case rvc_inf:
     case rvc_nan:
+      /* Make resulting NaN value to be qNaN. The caller has the
+         responsibility to avoid the operation if flag_signaling_nans
+         is on.  */
+      r->signalling = 0;
       break;

     case rvc_normal:
Index: gcc/simplify-rtx.c
===================================================================
--- gcc/simplify-rtx.c  (revision 228700)
+++ gcc/simplify-rtx.c  (working copy)
@@ -1801,16 +1801,25 @@  simplify_const_unary_operation (enum rtx_code code
          d = real_value_negate (&d);
          break;
        case FLOAT_TRUNCATE:
-         d = real_value_truncate (mode, d);
+          /* Don't perform the operation if flag_signaling_nans is on
+             and the operand is a NaN.  */
+         if (!(HONOR_SNANS (mode) && REAL_VALUE_ISNAN (d)))
+           d = real_value_truncate (mode, d);
          break;
        case FLOAT_EXTEND:
          /* All this does is change the mode, unless changing
             mode class.  */
-         if (GET_MODE_CLASS (mode) != GET_MODE_CLASS (GET_MODE (op)))
+          /* Don't perform the operation if flag_signaling_nans is on
+             and the operand is a NaN.  */
+         if (GET_MODE_CLASS (mode) != GET_MODE_CLASS (GET_MODE (op))
+              && !(HONOR_SNANS (mode) && REAL_VALUE_ISNAN (d)))
            real_convert (&d, mode, &d);
          break;
        case FIX:
-         real_arithmetic (&d, FIX_TRUNC_EXPR, &d, NULL);
+          /* Don't perform the operation if flag_signaling_nans is on
+             and the operand is a NaN.  */
+         if (!(HONOR_SNANS (mode) && REAL_VALUE_ISNAN (d)))
+           real_arithmetic (&d, FIX_TRUNC_EXPR, &d, NULL);
          break;
        case NOT:
          {
Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c    (revision 228700)
+++ gcc/tree-ssa-math-opts.c    (working copy)
@@ -1486,6 +1486,13 @@  gimple_expand_builtin_pow (gimple_stmt_iterator *g
   if (TREE_CODE (arg1) != REAL_CST)
     return NULL_TREE;

+  /* Don't perform the operation if flag_signaling_nans is on
+     and the operand is a NaN.  */
+  if (HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg1)))
+      && (REAL_VALUE_ISNAN (TREE_REAL_CST (arg0))
+          || REAL_VALUE_ISNAN (TREE_REAL_CST (arg1))))
+    return NULL_TREE;
+
   /* If the exponent is equivalent to an integer, expand to an optimal
      multiplication sequence when profitable.  */
   c = TREE_REAL_CST (arg1);
Index: gcc/testsuite/gcc.dg/pr61441.c
===================================================================
--- gcc/testsuite/gcc.dg/pr61441.c      (revision 0)
+++ gcc/testsuite/gcc.dg/pr61441.c      (working copy)
@@ -0,0 +1,61 @@ 
+/* { dg-do run } */
+/* { dg-options "-O1 -lm" } */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <math.h>
+
+void conversion()
+{
+  float sNaN = __builtin_nansf ("");
+  double x = (double) sNaN;
+  if (issignaling(x))
+  {
+    __builtin_abort();
+  }
+}
+
+enum op {Add, Mult, Div, Abs};
+
+void operation(enum op t)
+{
+  float x, y;
+  float sNaN = __builtin_nansf ("");
+  switch (t)
+  {
+    case Abs:
+      x = fabs(sNaN);
+      break;
+    case Add:
+      x = sNaN + 2.0;
+      break;
+    case Mult:
+      x = sNaN * 2.0;
+      break;
+    case Div:
+    default:
+      x = sNaN / 2.0;
+      break;
+  }
+  if (t == Abs)
+  {
+    if (!issignaling(x))
+    {
+      __builtin_abort();
+    }
+  }
+  else if (issignaling(x))
+  {
+    __builtin_abort();
+  }
+}
+
+int main (void)
+{
+  conversion();
+  operation(Add);
+  operation(Mult);
+  operation(Div);
+  operation(Abs);
+  return 0;
+}