diff mbox

Fix 61441

Message ID CA+ZXfhVZx61N50nydQfDtMdD77329efLKpYmw+7g0rXvRHKvjQ@mail.gmail.com
State New
Headers show

Commit Message

Sujoy Saraswati Sept. 10, 2015, 7:30 a.m. UTC 
Hi,
 Here is a modified patch for this. The change this time is in
fold-const.c and real.c.

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-09-10  Sujoy Saraswati <ssaraswati@gmail.com>

        PR tree-optimization/61441
        * fold-const.c (const_binop, fold_abs_const): Convert
        sNaN to qNaN when flag_signaling_nans is off.
        * real.c (do_add, do_multiply, do_divide, do_fix_trunc): Same.
        (real_arithmetic, real_ldexp, real_convert): Same

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


On Wed, Sep 2, 2015 at 5:26 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Sep 2, 2015 at 1:36 PM, Sujoy Saraswati <ssaraswati@gmail.com> wrote:
>> Hi Richard,
>>
>>> Note that I'm curious what
>>> the actual bug is - is it that (double) sNaN creates a sNaN?  Then the fix
>>> should be elsewhere, in constant folding itself
>>> (fold_convert_const_real_from_real
>>> or real_convert).
>>>
>>> If that isn't the bug you have very many other passes to fix for the
>>> same problem.
>>>
>>> So - can you please explain?
>>
>> In this test case, the floating point operation for converting the
>> float to double is what should convert the sNaN to qNaN. I tried to
>> cover more floating point operations than just the conversion
>> operations exposed by this test case. For example, if we consider the
>> following program -
>>
>> #define _GNU_SOURCE
>> #include <stdio.h>
>> #include <math.h>
>>
>> int main (void)
>> {
>>   float x;
>>   float sNaN = __builtin_nansf ("");
>>   x = sNaN + .0;
>>   return issignaling(x);
>> }
>>
>> The operation (sNaN + .0) should also result in qNaN after folding.
>> Hence, I thought of doing the sNaN to qNaN conversion in various
>> places under tree-ssa-ccp, where the result upon a folding is
>> available. I do agree that this approach may mean many more such
>> places should also be covered in other passes, but thought of sending
>> the fix for the ccp pass to start with.
>>
>> Let me know if you suggest alternate approach.
>
> CCP and other passes ultimatively end up using fold-const.c:const_{unop,binop}
> for constant folding so that is where the fix should go to (or to real.c).  That
> will automatically handle other passes doing similar transforms.
>
> Richard.
>
>> Regards,
>> Sujoy
>>
>>> Thanks,
>>> Richard.
>>>
>>>> Regards,
>>>> Sujoy
>>>>
>>>> 2015-09-01  Sujoy Saraswati <ssaraswati@gmail.com>
>>>>
>>>>         PR tree-optimization/61441
>>>>         * tree-ssa-ccp.c (convert_snan_to_qnan): Convert sNaN to qNaN when
>>>>         flag_signaling_nans is off.
>>>>         (ccp_fold_stmt, visit_assignment, visit_cond_stmt): call
>>>>         convert_snan_to_qnan to convert sNaN to qNaN on constant folding.
>>>>
>>>>         PR tree-optimization/61441
>>>>         * gcc.dg/pr61441.c: New testcase.
>>>>
>>>> Index: gcc/tree-ssa-ccp.c
>>>> ===================================================================
>>>> --- gcc/tree-ssa-ccp.c  (revision 226965)
>>>> +++ gcc/tree-ssa-ccp.c  (working copy)
>>>> @@ -560,6 +560,24 @@ value_to_wide_int (ccp_prop_value_t val)
>>>>    return 0;
>>>>  }
>>>>
>>>> +/* Convert sNaN to qNaN when flag_signaling_nans is off */
>>>> +
>>>> +static void
>>>> +convert_snan_to_qnan (tree expr)
>>>> +{
>>>> +  if (expr
>>>> +      && (TREE_CODE (expr) == REAL_CST)
>>>> +      && !flag_signaling_nans)
>>>> +  {
>>>> +    REAL_VALUE_TYPE *d = TREE_REAL_CST_PTR (expr);
>>>> +
>>>> +    if (HONOR_NANS (TYPE_MODE (TREE_TYPE (expr)))
>>>> +        && REAL_VALUE_ISNAN (*d)
>>>> +        && d->signalling)
>>>> +      d->signalling = 0;
>>>> +  }
>>>> +}
>>>> +
>>>>  /* Return the value for the address expression EXPR based on alignment
>>>>     information.  */
>>>>
>>>> @@ -2156,6 +2174,7 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>>>>         if (val.lattice_val != CONSTANT
>>>>             || val.mask != 0)
>>>>           return false;
>>>> +        convert_snan_to_qnan (val.value);
>>>>
>>>>         if (dump_file)
>>>>           {
>>>> @@ -2197,7 +2216,10 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>>>>             bool res;
>>>>             if (!useless_type_conversion_p (TREE_TYPE (lhs),
>>>>                                             TREE_TYPE (new_rhs)))
>>>> +            {
>>>>               new_rhs = fold_convert (TREE_TYPE (lhs), new_rhs);
>>>> +              convert_snan_to_qnan (new_rhs);
>>>> +            }
>>>>             res = update_call_from_tree (gsi, new_rhs);
>>>>             gcc_assert (res);
>>>>             return true;
>>>> @@ -2216,6 +2238,7 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>>>>              tree new_rhs = fold_builtin_alloca_with_align (stmt);
>>>>              if (new_rhs)
>>>>               {
>>>> +                convert_snan_to_qnan (new_rhs);
>>>>                 bool res = update_call_from_tree (gsi, new_rhs);
>>>>                 tree var = TREE_OPERAND (TREE_OPERAND (new_rhs, 0),0);
>>>>                 gcc_assert (res);
>>>> @@ -2260,7 +2283,10 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>>>>           {
>>>>             tree rhs = unshare_expr (val);
>>>>             if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
>>>> +            {
>>>>               rhs = fold_build1 (VIEW_CONVERT_EXPR, TREE_TYPE (lhs), rhs);
>>>> +              convert_snan_to_qnan (rhs);
>>>> +            }
>>>>             gimple_assign_set_rhs_from_tree (gsi, rhs);
>>>>             return true;
>>>>           }
>>>> @@ -2292,6 +2318,7 @@ visit_assignment (gimple stmt, tree *output_p)
>>>>        /* Evaluate the statement, which could be
>>>>          either a GIMPLE_ASSIGN or a GIMPLE_CALL.  */
>>>>        val = evaluate_stmt (stmt);
>>>> +      convert_snan_to_qnan (val.value);
>>>>
>>>>        /* If STMT is an assignment to an SSA_NAME, we only have one
>>>>          value to set.  */
>>>> @@ -2324,6 +2351,7 @@ visit_cond_stmt (gimple stmt, edge *taken_edge_p)
>>>>    if (val.lattice_val != CONSTANT
>>>>        || val.mask != 0)
>>>>      return SSA_PROP_VARYING;
>>>> +  convert_snan_to_qnan (val.value);
>>>>
>>>>    /* Find which edge out of the conditional block will be taken and add it
>>>>       to the worklist.  If no single edge can be determined statically,
>>>>
>>>> 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,17 @@
>>>> +/* { dg-do run } */
>>>> +/* { dg-options "-O1 -lm" } */
>>>> +
>>>> +#define _GNU_SOURCE
>>>> +#include <stdio.h>
>>>> +#include <math.h>
>>>> +
>>>> +int main (void)
>>>> +{
>>>> +  float sNaN = __builtin_nansf ("");
>>>> +  double x = (double) sNaN;
>>>> +  if (issignaling(x))
>>>> +  {
>>>> +    __builtin_abort();
>>>> +  }
>>>> +  return 0;
>>>> +}

Comments

Richard Biener Sept. 14, 2015, 1:41 p.m. UTC | #1 
On Thu, Sep 10, 2015 at 9:30 AM, Sujoy Saraswati <ssaraswati@gmail.com> wrote:
> Hi,
>  Here is a modified patch for this. The change this time is in
> fold-const.c and real.c.
>
> 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-09-10  Sujoy Saraswati <ssaraswati@gmail.com>
>
>         PR tree-optimization/61441
>         * fold-const.c (const_binop, fold_abs_const): Convert
>         sNaN to qNaN when flag_signaling_nans is off.
>         * real.c (do_add, do_multiply, do_divide, do_fix_trunc): Same.
>         (real_arithmetic, real_ldexp, real_convert): Same
>
>         PR tree-optimization/61441
>         * gcc.dg/pr61441.c: New testcase.
>
> Index: gcc/fold-const.c
> ===================================================================
> --- gcc/fold-const.c    (revision 227584)
> +++ gcc/fold-const.c    (working copy)
> @@ -1183,9 +1183,19 @@ 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))
> +      {
> +        /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +        if (!flag_signaling_nans)
> +          (TREE_REAL_CST_PTR (arg1))->signalling = 0;

As REAL_CSTs can be shared you need to build a new one, you can't
modify it in place.

I'll leave the correctness part of the patch to Joseph who knows FP
arithmetic better than me,
implementation-wise this is ok if you fix the REAL_CST sharing issue.

Thanks,
Richard.

>         return arg1;
> +      }
>        else if (REAL_VALUE_ISNAN (d2))
> +      {
> +        /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +        if (!flag_signaling_nans)
> +          (TREE_REAL_CST_PTR (arg1))->signalling = 0;
>         return arg2;
> +      }
>
>        inexact = real_arithmetic (&value, code, &d1, &d2);
>        real_convert (&result, mode, &value);
> @@ -13644,6 +13654,9 @@ fold_abs_const (tree arg0, tree type)
>         t = build_real (type, real_value_negate (&TREE_REAL_CST (arg0)));
>        else
>         t =  arg0;
> +      /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +      if (!flag_signaling_nans)
> +        (TREE_REAL_CST_PTR (t))->signalling = 0;
>        break;
>
>      default:
> Index: gcc/real.c
> ===================================================================
> --- gcc/real.c  (revision 227584)
> +++ gcc/real.c  (working copy)
> @@ -545,6 +545,9 @@ do_add (REAL_VALUE_TYPE *r, const REAL_VALUE_TYPE
>      case CLASS2 (rvc_normal, rvc_inf):
>        /* R + Inf = Inf.  */
>        *r = *b;
> +      /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +      if (!flag_signaling_nans)
> +        r->signalling = 0;
>        r->sign = sign ^ subtract_p;
>        return false;
>
> @@ -558,6 +561,9 @@ do_add (REAL_VALUE_TYPE *r, const REAL_VALUE_TYPE
>      case CLASS2 (rvc_inf, rvc_normal):
>        /* Inf + R = Inf.  */
>        *r = *a;
> +      /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +      if (!flag_signaling_nans)
> +        r->signalling = 0;
>        return false;
>
>      case CLASS2 (rvc_inf, rvc_inf):
> @@ -680,6 +686,9 @@ do_multiply (REAL_VALUE_TYPE *r, const REAL_VALUE_
>      case CLASS2 (rvc_nan, rvc_nan):
>        /* ANY * NaN = NaN.  */
>        *r = *b;
> +      /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +      if (!flag_signaling_nans)
> +        r->signalling = 0;
>        r->sign = sign;
>        return false;
>
> @@ -688,6 +697,9 @@ do_multiply (REAL_VALUE_TYPE *r, const REAL_VALUE_
>      case CLASS2 (rvc_nan, rvc_inf):
>        /* NaN * ANY = NaN.  */
>        *r = *a;
> +      /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +      if (!flag_signaling_nans)
> +        r->signalling = 0;
>        r->sign = sign;
>        return false;
>
> @@ -830,6 +842,9 @@ do_divide (REAL_VALUE_TYPE *r, const REAL_VALUE_TY
>      case CLASS2 (rvc_nan, rvc_nan):
>        /* ANY / NaN = NaN.  */
>        *r = *b;
> +      /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +      if (!flag_signaling_nans)
> +        r->signalling = 0;
>        r->sign = sign;
>        return false;
>
> @@ -838,6 +853,9 @@ do_divide (REAL_VALUE_TYPE *r, const REAL_VALUE_TY
>      case CLASS2 (rvc_nan, rvc_inf):
>        /* NaN / ANY = NaN.  */
>        *r = *a;
> +      /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +      if (!flag_signaling_nans)
> +        r->signalling = 0;
>        r->sign = sign;
>        return false;
>
> @@ -968,6 +986,9 @@ do_fix_trunc (REAL_VALUE_TYPE *r, const REAL_VALUE
>      case rvc_zero:
>      case rvc_inf:
>      case rvc_nan:
> +      /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +      if (!flag_signaling_nans)
> +        r->signalling = 0;
>        break;
>
>      case rvc_normal:
> @@ -1059,6 +1080,11 @@ real_arithmetic (REAL_VALUE_TYPE *r, int icode, co
>      default:
>        gcc_unreachable ();
>      }
> +
> +  /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +  if (!flag_signaling_nans)
> +    r->signalling = 0;
> +
>    return false;
>  }
>
> @@ -1150,6 +1176,9 @@ real_ldexp (REAL_VALUE_TYPE *r, const REAL_VALUE_T
>      case rvc_zero:
>      case rvc_inf:
>      case rvc_nan:
> +      /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +      if (!flag_signaling_nans)
> +        r->signalling = 0;
>        break;
>
>      case rvc_normal:
> @@ -2718,6 +2747,10 @@ real_convert (REAL_VALUE_TYPE *r, machine_mode mod
>
>    round_for_format (fmt, r);
>
> +  /* Convert sNaN to qNaN when flag_signaling_nans is off */
> +  if (HONOR_NANS (mode) && !flag_signaling_nans)
> +    r->signalling = 0;
> +
>    /* round_for_format de-normalizes denormals.  Undo just that part.  */
>    if (r->cl == rvc_normal)
>      normalize (r);
> 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,34 @@
> +/* { 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();
> +  }
> +}
> +
> +void addition()
> +{
> +  float x;
> +  float sNaN = __builtin_nansf ("");
> +  x = sNaN + .0;
> +  if (issignaling(x))
> +  {
> +    __builtin_abort();
> +  }
> +}
> +
> +int main (void)
> +{
> +  conversion();
> +  addition();
> +  return 0;
> +}
>
> On Wed, Sep 2, 2015 at 5:26 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Sep 2, 2015 at 1:36 PM, Sujoy Saraswati <ssaraswati@gmail.com> wrote:
>>> Hi Richard,
>>>
>>>> Note that I'm curious what
>>>> the actual bug is - is it that (double) sNaN creates a sNaN?  Then the fix
>>>> should be elsewhere, in constant folding itself
>>>> (fold_convert_const_real_from_real
>>>> or real_convert).
>>>>
>>>> If that isn't the bug you have very many other passes to fix for the
>>>> same problem.
>>>>
>>>> So - can you please explain?
>>>
>>> In this test case, the floating point operation for converting the
>>> float to double is what should convert the sNaN to qNaN. I tried to
>>> cover more floating point operations than just the conversion
>>> operations exposed by this test case. For example, if we consider the
>>> following program -
>>>
>>> #define _GNU_SOURCE
>>> #include <stdio.h>
>>> #include <math.h>
>>>
>>> int main (void)
>>> {
>>>   float x;
>>>   float sNaN = __builtin_nansf ("");
>>>   x = sNaN + .0;
>>>   return issignaling(x);
>>> }
>>>
>>> The operation (sNaN + .0) should also result in qNaN after folding.
>>> Hence, I thought of doing the sNaN to qNaN conversion in various
>>> places under tree-ssa-ccp, where the result upon a folding is
>>> available. I do agree that this approach may mean many more such
>>> places should also be covered in other passes, but thought of sending
>>> the fix for the ccp pass to start with.
>>>
>>> Let me know if you suggest alternate approach.
>>
>> CCP and other passes ultimatively end up using fold-const.c:const_{unop,binop}
>> for constant folding so that is where the fix should go to (or to real.c).  That
>> will automatically handle other passes doing similar transforms.
>>
>> Richard.
>>
>>> Regards,
>>> Sujoy
>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> Regards,
>>>>> Sujoy
>>>>>
>>>>> 2015-09-01  Sujoy Saraswati <ssaraswati@gmail.com>
>>>>>
>>>>>         PR tree-optimization/61441
>>>>>         * tree-ssa-ccp.c (convert_snan_to_qnan): Convert sNaN to qNaN when
>>>>>         flag_signaling_nans is off.
>>>>>         (ccp_fold_stmt, visit_assignment, visit_cond_stmt): call
>>>>>         convert_snan_to_qnan to convert sNaN to qNaN on constant folding.
>>>>>
>>>>>         PR tree-optimization/61441
>>>>>         * gcc.dg/pr61441.c: New testcase.
>>>>>
>>>>> Index: gcc/tree-ssa-ccp.c
>>>>> ===================================================================
>>>>> --- gcc/tree-ssa-ccp.c  (revision 226965)
>>>>> +++ gcc/tree-ssa-ccp.c  (working copy)
>>>>> @@ -560,6 +560,24 @@ value_to_wide_int (ccp_prop_value_t val)
>>>>>    return 0;
>>>>>  }
>>>>>
>>>>> +/* Convert sNaN to qNaN when flag_signaling_nans is off */
>>>>> +
>>>>> +static void
>>>>> +convert_snan_to_qnan (tree expr)
>>>>> +{
>>>>> +  if (expr
>>>>> +      && (TREE_CODE (expr) == REAL_CST)
>>>>> +      && !flag_signaling_nans)
>>>>> +  {
>>>>> +    REAL_VALUE_TYPE *d = TREE_REAL_CST_PTR (expr);
>>>>> +
>>>>> +    if (HONOR_NANS (TYPE_MODE (TREE_TYPE (expr)))
>>>>> +        && REAL_VALUE_ISNAN (*d)
>>>>> +        && d->signalling)
>>>>> +      d->signalling = 0;
>>>>> +  }
>>>>> +}
>>>>> +
>>>>>  /* Return the value for the address expression EXPR based on alignment
>>>>>     information.  */
>>>>>
>>>>> @@ -2156,6 +2174,7 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>>>>>         if (val.lattice_val != CONSTANT
>>>>>             || val.mask != 0)
>>>>>           return false;
>>>>> +        convert_snan_to_qnan (val.value);
>>>>>
>>>>>         if (dump_file)
>>>>>           {
>>>>> @@ -2197,7 +2216,10 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>>>>>             bool res;
>>>>>             if (!useless_type_conversion_p (TREE_TYPE (lhs),
>>>>>                                             TREE_TYPE (new_rhs)))
>>>>> +            {
>>>>>               new_rhs = fold_convert (TREE_TYPE (lhs), new_rhs);
>>>>> +              convert_snan_to_qnan (new_rhs);
>>>>> +            }
>>>>>             res = update_call_from_tree (gsi, new_rhs);
>>>>>             gcc_assert (res);
>>>>>             return true;
>>>>> @@ -2216,6 +2238,7 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>>>>>              tree new_rhs = fold_builtin_alloca_with_align (stmt);
>>>>>              if (new_rhs)
>>>>>               {
>>>>> +                convert_snan_to_qnan (new_rhs);
>>>>>                 bool res = update_call_from_tree (gsi, new_rhs);
>>>>>                 tree var = TREE_OPERAND (TREE_OPERAND (new_rhs, 0),0);
>>>>>                 gcc_assert (res);
>>>>> @@ -2260,7 +2283,10 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>>>>>           {
>>>>>             tree rhs = unshare_expr (val);
>>>>>             if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
>>>>> +            {
>>>>>               rhs = fold_build1 (VIEW_CONVERT_EXPR, TREE_TYPE (lhs), rhs);
>>>>> +              convert_snan_to_qnan (rhs);
>>>>> +            }
>>>>>             gimple_assign_set_rhs_from_tree (gsi, rhs);
>>>>>             return true;
>>>>>           }
>>>>> @@ -2292,6 +2318,7 @@ visit_assignment (gimple stmt, tree *output_p)
>>>>>        /* Evaluate the statement, which could be
>>>>>          either a GIMPLE_ASSIGN or a GIMPLE_CALL.  */
>>>>>        val = evaluate_stmt (stmt);
>>>>> +      convert_snan_to_qnan (val.value);
>>>>>
>>>>>        /* If STMT is an assignment to an SSA_NAME, we only have one
>>>>>          value to set.  */
>>>>> @@ -2324,6 +2351,7 @@ visit_cond_stmt (gimple stmt, edge *taken_edge_p)
>>>>>    if (val.lattice_val != CONSTANT
>>>>>        || val.mask != 0)
>>>>>      return SSA_PROP_VARYING;
>>>>> +  convert_snan_to_qnan (val.value);
>>>>>
>>>>>    /* Find which edge out of the conditional block will be taken and add it
>>>>>       to the worklist.  If no single edge can be determined statically,
>>>>>
>>>>> 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,17 @@
>>>>> +/* { dg-do run } */
>>>>> +/* { dg-options "-O1 -lm" } */
>>>>> +
>>>>> +#define _GNU_SOURCE
>>>>> +#include <stdio.h>
>>>>> +#include <math.h>
>>>>> +
>>>>> +int main (void)
>>>>> +{
>>>>> +  float sNaN = __builtin_nansf ("");
>>>>> +  double x = (double) sNaN;
>>>>> +  if (issignaling(x))
>>>>> +  {
>>>>> +    __builtin_abort();
>>>>> +  }
>>>>> +  return 0;
>>>>> +}
Joseph Myers Sept. 14, 2015, 8:37 p.m. UTC | #2 
On Mon, 14 Sep 2015, Richard Biener wrote:

> I'll leave the correctness part of the patch to Joseph who knows FP
> arithmetic better than me,
> implementation-wise this is ok if you fix the REAL_CST sharing issue.

Changing fold_abs_const is wrong - abs of sNaN is sNaN, no exceptions 
raised.  Changing real_arithmetic is wrong for the NEGATE_EXPR and 
ABS_EXPR cases, both of which should just affect the sign bit without 
quieting sNaNs.

All the comments in the patch should end with ".  " (full stop, two 
spaces).

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).

Inside static initializers, expressions involving sNaNs still need to be 
folded (so sNaN + 1 becomes qNaN inside such an initializer, for example, 
with the translation-time exception being discarded).  Again, existing 
code should handle this: START_FOLD_INIT / END_FOLD_INIT already handle 
clearing and restoring flag_signaling_nans.

My understanding of the design of the existing code is that real.c will do 
the arithmetic regardless of whether it might raise an exception or have 
rounding-mode-dependent results, with fold-const.c being responsible for 
deciding whether the result can be used to fold the expression in 
question.  That is, you shouldn't need any flag_signaling_nans conditions 
in real.c; rather, if IEEE semantics mean an sNaN is quieted, real.c 
should do so unconditionally.  It should be the callers in fold-const.c 
that check HONOR_SNANS and disallow folding when it would lose exceptions.
Sujoy Saraswati Sept. 16, 2015, 12:41 p.m. UTC | #3 
Hi,

>> I'll leave the correctness part of the patch to Joseph who knows FP
>> arithmetic better than me,
>> implementation-wise this is ok if you fix the REAL_CST sharing issue.

Ok, will change this.

> Changing fold_abs_const is wrong - abs of sNaN is sNaN, no exceptions
> raised.  Changing real_arithmetic is wrong for the NEGATE_EXPR and
> ABS_EXPR cases, both of which should just affect the sign bit without
> quieting sNaNs.

Right, I had thought unary operators will be similar to binary
operators for this. Will fix this.

> All the comments in the patch should end with ".  " (full stop, two
> spaces).

Ok.

> 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.

> Inside static initializers, expressions involving sNaNs still need to be
> folded (so sNaN + 1 becomes qNaN inside such an initializer, for example,
> with the translation-time exception being discarded).  Again, existing
> code should handle this: START_FOLD_INIT / END_FOLD_INIT already handle
> clearing and restoring flag_signaling_nans.

Yes.

> My understanding of the design of the existing code is that real.c will do
> the arithmetic regardless of whether it might raise an exception or have
> rounding-mode-dependent results, with fold-const.c being responsible for
> deciding whether the result can be used to fold the expression in
> question.  That is, you shouldn't need any flag_signaling_nans conditions
> in real.c; rather, if IEEE semantics mean an sNaN is quieted, real.c
> should do so unconditionally.  It should be the callers in fold-const.c
> that check HONOR_SNANS and disallow folding when it would lose exceptions.

I had added the flag_signaling_nans condition within real.c so that
the callers can do the folding, with the sNaN value converted to qNaN.
However, as you mention, the real.c design should do this irrespective
of the signaling flag and leave it for the callers to disallow the
folding.

I will make the modifications and post a patch.

Regards,
Sujoy

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers Sept. 16, 2015, 4:39 p.m. UTC | #4 
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).
diff mbox

Patch

Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c    (revision 227584)
+++ gcc/fold-const.c    (working copy)
@@ -1183,9 +1183,19 @@  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))
+      {
+        /* Convert sNaN to qNaN when flag_signaling_nans is off */
+        if (!flag_signaling_nans)
+          (TREE_REAL_CST_PTR (arg1))->signalling = 0;
        return arg1;
+      }
       else if (REAL_VALUE_ISNAN (d2))
+      {
+        /* Convert sNaN to qNaN when flag_signaling_nans is off */
+        if (!flag_signaling_nans)
+          (TREE_REAL_CST_PTR (arg1))->signalling = 0;
        return arg2;
+      }

       inexact = real_arithmetic (&value, code, &d1, &d2);
       real_convert (&result, mode, &value);
@@ -13644,6 +13654,9 @@  fold_abs_const (tree arg0, tree type)
        t = build_real (type, real_value_negate (&TREE_REAL_CST (arg0)));
       else
        t =  arg0;
+      /* Convert sNaN to qNaN when flag_signaling_nans is off */
+      if (!flag_signaling_nans)
+        (TREE_REAL_CST_PTR (t))->signalling = 0;
       break;

     default:
Index: gcc/real.c
===================================================================
--- gcc/real.c  (revision 227584)
+++ gcc/real.c  (working copy)
@@ -545,6 +545,9 @@  do_add (REAL_VALUE_TYPE *r, const REAL_VALUE_TYPE
     case CLASS2 (rvc_normal, rvc_inf):
       /* R + Inf = Inf.  */
       *r = *b;
+      /* Convert sNaN to qNaN when flag_signaling_nans is off */
+      if (!flag_signaling_nans)
+        r->signalling = 0;
       r->sign = sign ^ subtract_p;
       return false;

@@ -558,6 +561,9 @@  do_add (REAL_VALUE_TYPE *r, const REAL_VALUE_TYPE
     case CLASS2 (rvc_inf, rvc_normal):
       /* Inf + R = Inf.  */
       *r = *a;
+      /* Convert sNaN to qNaN when flag_signaling_nans is off */
+      if (!flag_signaling_nans)
+        r->signalling = 0;
       return false;

     case CLASS2 (rvc_inf, rvc_inf):
@@ -680,6 +686,9 @@  do_multiply (REAL_VALUE_TYPE *r, const REAL_VALUE_
     case CLASS2 (rvc_nan, rvc_nan):
       /* ANY * NaN = NaN.  */
       *r = *b;
+      /* Convert sNaN to qNaN when flag_signaling_nans is off */
+      if (!flag_signaling_nans)
+        r->signalling = 0;
       r->sign = sign;
       return false;

@@ -688,6 +697,9 @@  do_multiply (REAL_VALUE_TYPE *r, const REAL_VALUE_
     case CLASS2 (rvc_nan, rvc_inf):
       /* NaN * ANY = NaN.  */
       *r = *a;
+      /* Convert sNaN to qNaN when flag_signaling_nans is off */
+      if (!flag_signaling_nans)
+        r->signalling = 0;
       r->sign = sign;
       return false;

@@ -830,6 +842,9 @@  do_divide (REAL_VALUE_TYPE *r, const REAL_VALUE_TY
     case CLASS2 (rvc_nan, rvc_nan):
       /* ANY / NaN = NaN.  */
       *r = *b;
+      /* Convert sNaN to qNaN when flag_signaling_nans is off */
+      if (!flag_signaling_nans)
+        r->signalling = 0;
       r->sign = sign;
       return false;

@@ -838,6 +853,9 @@  do_divide (REAL_VALUE_TYPE *r, const REAL_VALUE_TY
     case CLASS2 (rvc_nan, rvc_inf):
       /* NaN / ANY = NaN.  */
       *r = *a;
+      /* Convert sNaN to qNaN when flag_signaling_nans is off */
+      if (!flag_signaling_nans)
+        r->signalling = 0;
       r->sign = sign;
       return false;

@@ -968,6 +986,9 @@  do_fix_trunc (REAL_VALUE_TYPE *r, const REAL_VALUE
     case rvc_zero:
     case rvc_inf:
     case rvc_nan:
+      /* Convert sNaN to qNaN when flag_signaling_nans is off */
+      if (!flag_signaling_nans)
+        r->signalling = 0;
       break;

     case rvc_normal:
@@ -1059,6 +1080,11 @@  real_arithmetic (REAL_VALUE_TYPE *r, int icode, co
     default:
       gcc_unreachable ();
     }
+
+  /* Convert sNaN to qNaN when flag_signaling_nans is off */
+  if (!flag_signaling_nans)
+    r->signalling = 0;
+
   return false;
 }

@@ -1150,6 +1176,9 @@  real_ldexp (REAL_VALUE_TYPE *r, const REAL_VALUE_T
     case rvc_zero:
     case rvc_inf:
     case rvc_nan:
+      /* Convert sNaN to qNaN when flag_signaling_nans is off */
+      if (!flag_signaling_nans)
+        r->signalling = 0;
       break;

     case rvc_normal:
@@ -2718,6 +2747,10 @@  real_convert (REAL_VALUE_TYPE *r, machine_mode mod

   round_for_format (fmt, r);

+  /* Convert sNaN to qNaN when flag_signaling_nans is off */
+  if (HONOR_NANS (mode) && !flag_signaling_nans)
+    r->signalling = 0;
+
   /* round_for_format de-normalizes denormals.  Undo just that part.  */
   if (r->cl == rvc_normal)
     normalize (r);
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,34 @@ 
+/* { 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();
+  }
+}
+
+void addition()
+{
+  float x;
+  float sNaN = __builtin_nansf ("");
+  x = sNaN + .0;
+  if (issignaling(x))
+  {
+    __builtin_abort();
+  }
+}
+
+int main (void)
+{
+  conversion();
+  addition();
+  return 0;
+}