[PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Jakub Jelinek]

Hi!

The testcase in the PR
constexpr bool a = __builtin_nan ("") > 0.0;
constexpr bool b = __builtin_nans ("") > 0.0;
constexpr bool c = __builtin_nan ("") < 0.0;
constexpr bool d = __builtin_nans ("") < 0.0;
constexpr bool e = __builtin_nan ("") >= 0.0;
constexpr bool f = __builtin_nans ("") >= 0.0;
constexpr bool g = __builtin_nan ("") <= 0.0;
constexpr bool h = __builtin_nans ("") <= 0.0;
has inconsistent behavior, we fold c and d initializers to 0 and don't fold
any other comparisons to zero.
Not including the testcase in the testsuite because I really don't know
if it should be accepted or rejected (it is all accepted with
-fno-trapping-math).
The reason we optimize those < 0.0 comparisons to 0 is:
      /* Convert ABS_EXPR<x> < 0 to false.  */
      strict_overflow_p = false;
      if (code == LT_EXPR
          && (integer_zerop (arg1) || real_zerop (arg1))
          && tree_expr_nonnegative_warnv_p (arg0, &strict_overflow_p))
and we return true for __builtin_nan ("") (but not for -__builtin_nan ("").

Now, it just feels wrong to me to say that NaN with sign bit clear is
non-negative, but other than the above inconsistency I haven't been able to
construct a miscompiled testcase, only questionable thing is that
we fold away also comparisons of sNaN < 0.0 - but then while for sNaN >= 0.0
we don't fold that away during gimple optimizations, we fold it during RTL
optimizations at least on x86_64.

So, I really don't know if we want this or not, posting it for discussions.

2020-11-26  Jakub Jelinek  <jakub@redhat.com>

	PR c++/97965
	* fold-const.c (tree_single_nonnegative_warnv_p): Don't return true
	for NaNs with sign bit clear.

--- gcc/fold-const.c.jj	2020-11-24 09:02:25.330419895 +0100
+++ gcc/fold-const.c	2020-11-25 15:37:14.426229476 +0100
@@ -14186,7 +14186,9 @@ tree_single_nonnegative_warnv_p (tree t,
       return tree_int_cst_sgn (t) >= 0;
 
     case REAL_CST:
-      return ! REAL_VALUE_NEGATIVE (TREE_REAL_CST (t));
+      return (! REAL_VALUE_NEGATIVE (TREE_REAL_CST (t))
+	      /* Don't consider NaNs non-negative.  */
+	      && ! REAL_VALUE_ISNAN (TREE_REAL_CST (t)));
 
     case FIXED_CST:
       return ! FIXED_VALUE_NEGATIVE (TREE_FIXED_CST (t));

	Jakub

Re: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Richard Biener]

On Thu, 26 Nov 2020, Jakub Jelinek wrote:

> Hi!
> 
> The testcase in the PR
> constexpr bool a = __builtin_nan ("") > 0.0;
> constexpr bool b = __builtin_nans ("") > 0.0;
> constexpr bool c = __builtin_nan ("") < 0.0;
> constexpr bool d = __builtin_nans ("") < 0.0;
> constexpr bool e = __builtin_nan ("") >= 0.0;
> constexpr bool f = __builtin_nans ("") >= 0.0;
> constexpr bool g = __builtin_nan ("") <= 0.0;
> constexpr bool h = __builtin_nans ("") <= 0.0;
> has inconsistent behavior, we fold c and d initializers to 0 and don't fold
> any other comparisons to zero.
> Not including the testcase in the testsuite because I really don't know
> if it should be accepted or rejected (it is all accepted with
> -fno-trapping-math).
> The reason we optimize those < 0.0 comparisons to 0 is:
>       /* Convert ABS_EXPR<x> < 0 to false.  */
>       strict_overflow_p = false;
>       if (code == LT_EXPR
>           && (integer_zerop (arg1) || real_zerop (arg1))
>           && tree_expr_nonnegative_warnv_p (arg0, &strict_overflow_p))
> and we return true for __builtin_nan ("") (but not for -__builtin_nan ("").
> 
> Now, it just feels wrong to me to say that NaN with sign bit clear is
> non-negative, but other than the above inconsistency I haven't been able to
> construct a miscompiled testcase, only questionable thing is that
> we fold away also comparisons of sNaN < 0.0 - but then while for sNaN >= 0.0
> we don't fold that away during gimple optimizations, we fold it during RTL
> optimizations at least on x86_64.
> 
> So, I really don't know if we want this or not, posting it for discussions.

Is copysign (x, NaN) supposed to be well-defined?  We'd stop folding
this then, no?  I think the ABS_EXPR<x> < 0 to false folding is
simply incomplete and should first check whether the operands are
ordered?  That said, NaN is nonnegative but NaN < 0 isn't false(?)

So I don't think the patch is good.

Richard.

> 2020-11-26  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/97965
> 	* fold-const.c (tree_single_nonnegative_warnv_p): Don't return true
> 	for NaNs with sign bit clear.
> 
> --- gcc/fold-const.c.jj	2020-11-24 09:02:25.330419895 +0100
> +++ gcc/fold-const.c	2020-11-25 15:37:14.426229476 +0100
> @@ -14186,7 +14186,9 @@ tree_single_nonnegative_warnv_p (tree t,
>        return tree_int_cst_sgn (t) >= 0;
>  
>      case REAL_CST:
> -      return ! REAL_VALUE_NEGATIVE (TREE_REAL_CST (t));
> +      return (! REAL_VALUE_NEGATIVE (TREE_REAL_CST (t))
> +	      /* Don't consider NaNs non-negative.  */
> +	      && ! REAL_VALUE_ISNAN (TREE_REAL_CST (t)));
>  
>      case FIXED_CST:
>        return ! FIXED_VALUE_NEGATIVE (TREE_FIXED_CST (t));
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend

Re: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Jakub Jelinek]

On Thu, Nov 26, 2020 at 09:16:29AM +0000, Richard Biener wrote:
> > So, I really don't know if we want this or not, posting it for discussions.
> 
> Is copysign (x, NaN) supposed to be well-defined?  We'd stop folding
> this then, no?

Yes, we'd stop folding several cases with NaNs.

>  I think the ABS_EXPR<x> < 0 to false folding is
> simply incomplete and should first check whether the operands are
> ordered?  That said, NaN is nonnegative but NaN < 0 isn't false(?)
> 
> So I don't think the patch is good.

Another possibility (if we have this optimization already in match.pd too)
would be to only optimize the < 0 case in GENERIC if !HONOR_NANS like
the >= 0 case is and only optimize it in GIMPLE.  Though with the
default -ftrapping-math I think even optimizing qNaN < 0 to 0 is incorrect,
even that should raise invalid exception, shouldn't it?
So perhaps add a defaulted argument to the *nonnegative* APIs that would
say whether unordered is ok or not?

	Jakub

Re: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Richard Biener]

On Thu, 26 Nov 2020, Jakub Jelinek wrote:

> On Thu, Nov 26, 2020 at 09:16:29AM +0000, Richard Biener wrote:
> > > So, I really don't know if we want this or not, posting it for discussions.
> > 
> > Is copysign (x, NaN) supposed to be well-defined?  We'd stop folding
> > this then, no?
> 
> Yes, we'd stop folding several cases with NaNs.
> 
> >  I think the ABS_EXPR<x> < 0 to false folding is
> > simply incomplete and should first check whether the operands are
> > ordered?  That said, NaN is nonnegative but NaN < 0 isn't false(?)
> > 
> > So I don't think the patch is good.
> 
> Another possibility (if we have this optimization already in match.pd too)
> would be to only optimize the < 0 case in GENERIC if !HONOR_NANS like
> the >= 0 case is and only optimize it in GIMPLE.  Though with the
> default -ftrapping-math I think even optimizing qNaN < 0 to 0 is incorrect,
> even that should raise invalid exception, shouldn't it?
> So perhaps add a defaulted argument to the *nonnegative* APIs that would
> say whether unordered is ok or not?

Roger recently added some exhaustive changes in related areas, so let's
see if he has anything to say here.

Richard.

RE: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Roger Sayle]

[-- Attachment #1: Type: text/plain, Size: 2948 bytes --]


Many thanks for including me on this discussion.  It's extremely
interesting...

NaNs have a sign-bit, so copyexpr (and negate and ABS_EXPR) are
well-defined,
and it's reasonable for nonnegative_p to reflect this.  IMHO, the true bug
is that
we can't fold away (any) comparisons against NaN when flag_trapping_math,
irrespective of qNaN, -qNaN, sNaN or -sNaN.

My completely untested solution is the attached patch.  My apologies, I'm
not
even set up to compile things on the laptop that I'm composing this e-mail
on,
but my notes/proposals on tackling PR97965 are easier expressed as the
actual
suggested changes/edits.  [Forgive me if I've made a typo].

As Joseph correctly points out, signaling NaNs are a red herring here, as
both
sNaNs and qNaNs may trap during regular (ordered) comparisons.

The piece that I'll leave to the (C++) front-end folks to argue, is whether
constexpr
implies -fno-trapping-math like initializers, c.f.
START_FOLD_INIT/END_FOLD_INIT
in fold-const.c.  This controls whether the test case should be consistently
true or
consistently false, but the patches above address Jakub's concern in the PR
that
things should at least be consistent.

I hope this helps.  I'm happy to spin this patch myself but it may take a
little while.
Hopefully, this is sufficient to point folks in the right (or one possible)
direction.

Best regards,
Roger
--
Roger Sayle
NextMove Software Limited
Cambridge, UK

-----Original Message-----
From: Richard Biener <rguenther@suse.de> 
Sent: 26 November 2020 10:04
To: Jakub Jelinek <jakub@redhat.com>
Cc: Joseph S. Myers <joseph@codesourcery.com>; Jason Merrill
<jason@redhat.com>; gcc-patches@gcc.gnu.org; roger@nextmovesoftware.com
Subject: Re: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

On Thu, 26 Nov 2020, Jakub Jelinek wrote:

> On Thu, Nov 26, 2020 at 09:16:29AM +0000, Richard Biener wrote:
> > > So, I really don't know if we want this or not, posting it for
discussions.
> > 
> > Is copysign (x, NaN) supposed to be well-defined?  We'd stop folding 
> > this then, no?
> 
> Yes, we'd stop folding several cases with NaNs.
> 
> >  I think the ABS_EXPR<x> < 0 to false folding is simply incomplete 
> > and should first check whether the operands are ordered?  That said, 
> > NaN is nonnegative but NaN < 0 isn't false(?)
> > 
> > So I don't think the patch is good.
> 
> Another possibility (if we have this optimization already in match.pd 
> too) would be to only optimize the < 0 case in GENERIC if !HONOR_NANS 
> like the >= 0 case is and only optimize it in GIMPLE.  Though with the 
> default -ftrapping-math I think even optimizing qNaN < 0 to 0 is 
> incorrect, even that should raise invalid exception, shouldn't it?
> So perhaps add a defaulted argument to the *nonnegative* APIs that 
> would say whether unordered is ok or not?

Roger recently added some exhaustive changes in related areas, so let's see
if he has anything to say here.

Richard.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2377 bytes --]

diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 632a241a964..b76e80c02a3 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -12007,8 +12007,8 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
       strict_overflow_p = false;
       if (code == GE_EXPR
 	  && (integer_zerop (arg1)
-	      || (! HONOR_NANS (arg0)
-		  && real_zerop (arg1)))
+	      || (real_zerop (arg1)
+		  && !tree_expr_maybe_nan_p (arg0)))
 	  && tree_expr_nonnegative_warnv_p (arg0, &strict_overflow_p))
 	{
 	  if (strict_overflow_p)
@@ -12024,7 +12024,10 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
       /* Convert ABS_EXPR<x> < 0 to false.  */
       strict_overflow_p = false;
       if (code == LT_EXPR
-	  && (integer_zerop (arg1) || real_zerop (arg1))
+	  && (integer_zerop (arg1)
+	      || (real_zerop (arg1)
+		  && (!flag_trapping_math
+		      || !tree_expr_maybe_nan_p (arg0))))
 	  && tree_expr_nonnegative_warnv_p (arg0, &strict_overflow_p))
 	{
 	  if (strict_overflow_p)
diff --git a/gcc/match.pd b/gcc/match.pd
index f8b65154a9e..dec19ed2d57 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -3998,7 +3998,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
     (cmp @0 { build_real (TREE_TYPE (@1), dconst0); }))
    /* x != NaN is always true, other ops are always false.  */
    (if (REAL_VALUE_ISNAN (TREE_REAL_CST (@1))
-	&& ! HONOR_SNANS (@1))
+	&& ! flag_trapping_math)
     { constant_boolean_node (cmp == NE_EXPR, type); })
    /* Fold comparisons against infinity.  */
    (if (REAL_VALUE_ISINF (TREE_REAL_CST (@1))
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 47e7aebda8a..ab53570ac8c 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -5732,12 +5732,13 @@ simplify_const_relational_operation (enum rtx_code code,
       if (REAL_VALUE_ISNAN (*d0) || REAL_VALUE_ISNAN (*d1))
 	switch (code)
 	  {
+	  case NE:
+	    return flag_trapping_math ? 0 : const_true_rtx;
 	  case UNEQ:
 	  case UNLT:
 	  case UNGT:
 	  case UNLE:
 	  case UNGE:
-	  case NE:
 	  case UNORDERED:
 	    return const_true_rtx;
 	  case EQ:
@@ -5746,6 +5747,7 @@ simplify_const_relational_operation (enum rtx_code code,
 	  case LE:
 	  case GE:
 	  case LTGT:
+	    return flag_trapping_math ? 0 : const0_rtx;
 	  case ORDERED:
 	    return const0_rtx;
 	  default:

Re: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Jakub Jelinek]

On Thu, Nov 26, 2020 at 01:56:03PM -0000, Roger Sayle wrote:
> My completely untested solution is the attached patch.  My apologies, I'm
> not
> even set up to compile things on the laptop that I'm composing this e-mail
> on,
> but my notes/proposals on tackling PR97965 are easier expressed as the
> actual
> suggested changes/edits.  [Forgive me if I've made a typo].

Ah, thanks, I wasn't aware of that function.
Looking at the tree_expr_maybe_nan_p implementation, I wonder if:
    case PLUS_EXPR:
    case MINUS_EXPR:
    case MULT_EXPR:
      return !tree_expr_finite_p (TREE_OPERAND (x, 0))
             || !tree_expr_finite_p (TREE_OPERAND (x, 1));
shouldn't try harder, for + and minus, isn't
      return (tree_expr_maybe_nan_p (TREE_OPERAND (x, 0))
	      || tree_expr_maybe_nan_p (TREE_OPERAND (x, 1))
	      || (!tree_expr_finite_p (TREE_OPERAND (x, 0))
		  && !tree_expr_finite_p (TREE_OPERAND (x, 1))));
what we want to test?  I mean, if neither operand is a NaN and
one of the operands is finite, then the result will be either finite
or inf or -inf, but not NaN.  MULT_EXPR would presumably also need to
rule out zeros (i.e. use the *nonzero* APIs too, on the other side
+-inf * +-inf is not NaN).

Another thing, tree_expr_nonzero_warnv_p handles not just trees, but also
GIMPLE, shouldn't these tree_expr_finite_p and tree_expr_maybe_nan_p APIs
be also rewritten so that they can also handle SSA_NAMEs by walking the def
chains?

	Jakub

Re: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Jakub Jelinek]

On Thu, Nov 26, 2020 at 03:13:22PM +0100, Jakub Jelinek via Gcc-patches wrote:
> On Thu, Nov 26, 2020 at 01:56:03PM -0000, Roger Sayle wrote:
> > My completely untested solution is the attached patch.  My apologies, I'm
> > not
> > even set up to compile things on the laptop that I'm composing this e-mail
> > on,
> > but my notes/proposals on tackling PR97965 are easier expressed as the
> > actual
> > suggested changes/edits.  [Forgive me if I've made a typo].
> 
> Ah, thanks, I wasn't aware of that function.
> Looking at the tree_expr_maybe_nan_p implementation, I wonder if:
>     case PLUS_EXPR:
>     case MINUS_EXPR:
>     case MULT_EXPR:
>       return !tree_expr_finite_p (TREE_OPERAND (x, 0))
>              || !tree_expr_finite_p (TREE_OPERAND (x, 1));
> shouldn't try harder, for + and minus, isn't
>       return (tree_expr_maybe_nan_p (TREE_OPERAND (x, 0))
> 	      || tree_expr_maybe_nan_p (TREE_OPERAND (x, 1))
> 	      || (!tree_expr_finite_p (TREE_OPERAND (x, 0))
> 		  && !tree_expr_finite_p (TREE_OPERAND (x, 1))));
> what we want to test?  I mean, if neither operand is a NaN and
> one of the operands is finite, then the result will be either finite
> or inf or -inf, but not NaN.  MULT_EXPR would presumably also need to
> rule out zeros (i.e. use the *nonzero* APIs too, on the other side
> +-inf * +-inf is not NaN).
> 
> Another thing, tree_expr_nonzero_warnv_p handles not just trees, but also
> GIMPLE, shouldn't these tree_expr_finite_p and tree_expr_maybe_nan_p APIs
> be also rewritten so that they can also handle SSA_NAMEs by walking the def
> chains?

Though, of course, both of this can be done incrementally later on.

I'll test your patch tonight.

	Jakub

Re: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Jakub Jelinek]

On Thu, Nov 26, 2020 at 01:56:03PM -0000, Roger Sayle wrote:
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -3998,7 +3998,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>      (cmp @0 { build_real (TREE_TYPE (@1), dconst0); }))
>     /* x != NaN is always true, other ops are always false.  */
>     (if (REAL_VALUE_ISNAN (TREE_REAL_CST (@1))
> -	&& ! HONOR_SNANS (@1))
> +	&& ! flag_trapping_math)

Shouldn't this one stay as is for cmp == EQ_EXPR || cmp == NE_EXPR?
I mean, == or != only raise exceptions on sNaNs, while other simple
comparisons raise on both sNaNs and qNaNs.

> --- a/gcc/simplify-rtx.c
> +++ b/gcc/simplify-rtx.c
> @@ -5732,12 +5732,13 @@ simplify_const_relational_operation (enum rtx_code code,
>        if (REAL_VALUE_ISNAN (*d0) || REAL_VALUE_ISNAN (*d1))
>  	switch (code)
>  	  {
> +	  case NE:
> +	    return flag_trapping_math ? 0 : const_true_rtx;

And here too (for NE and would need moving EQ later too.

>  	  case UNEQ:
>  	  case UNLT:
>  	  case UNGT:
>  	  case UNLE:
>  	  case UNGE:
> -	  case NE:
>  	  case UNORDERED:
>  	    return const_true_rtx;
>  	  case EQ:
> @@ -5746,6 +5747,7 @@ simplify_const_relational_operation (enum rtx_code code,
>  	  case LE:
>  	  case GE:
>  	  case LTGT:
> +	    return flag_trapping_math ? 0 : const0_rtx;
>  	  case ORDERED:
>  	    return const0_rtx;
>  	  default:


	Jakub

RE: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Roger Sayle]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]


Hi Jakub,
Technically, PR97965 doesn't explicitly mention equality/inequality, but
you're
right, it makes sense to tackle this missed optimization at the same time as
we
fix the wrong-code.

On Thu, Nov 26, 2020, Jakub Jelinek wrote:
>On Thu, Nov 26, 2020 at 01:56:03PM -0000, Roger Sayle wrote:
>> --- a/gcc/match.pd
>> +++ b/gcc/match.pd
>> @@ -3998,7 +3998,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>>      (cmp @0 { build_real (TREE_TYPE (@1), dconst0); }))
>>     /* x != NaN is always true, other ops are always false.  */
>>     (if (REAL_VALUE_ISNAN (TREE_REAL_CST (@1))
>> -	&& ! HONOR_SNANS (@1))
>> +	&& ! flag_trapping_math)
>
>Shouldn't this one stay as is for cmp == EQ_EXPR || cmp == NE_EXPR?
>I mean, == or != only raise exceptions on sNaNs, while other simple
comparisons raise on both sNaNs and qNaNs.

The case to be careful of here is that (x == qNaN) can still raise/trap if x
is a sNaN.
Hence, the full condition should include:

&& (!flag_trapping_math
    || ((cmp == EQ_EXPR || cmp == NE_EXPR)
        && !tree_expr_maybe_signaling_nan_p (@1)
        && !tree_expr_maybe_signaling_nan_p (@0)))

Note: I repeat tree_expr_maybe_signaling_nan_p here for symmetry, though
because
we know @1 is a NaN, this could have been written as
!tree_expr_signaling_nan_p (@1).

>> --- a/gcc/simplify-rtx.c
>> +++ b/gcc/simplify-rtx.c
>> @@ -5732,12 +5732,13 @@ simplify_const_relational_operation (enum
rtx_code code,
>>        if (REAL_VALUE_ISNAN (*d0) || REAL_VALUE_ISNAN (*d1))
>>  	switch (code)
>>  	  {
>> +	  case NE:
>> +	    return flag_trapping_math ? 0 : const_true_rtx;
>
> And here too (for NE and would need moving EQ later too.

Yep/Agreed.  These cases become:

case NE:
        if (flag_trapping_math
            && (REAL_VALUE_ISSIGNALING_NAN (*d0)
                || REAL_VALUE_ISSIGANLING_NAN (*d1)))
          return 0;
        return const_true_rtx;

case EQ:
        if (flag_trapping_math
            && (REAL_VALUE_ISSIGNALING_NAN (*d0)
                || REAL_VALUE_ISSIGANLING_NAN (*d1)))
          return 0;
        return const0_rtx;


A revised patch is attached (which I've confirmed compiles but
haven't regression tested).

Many thanks again.

Roger
--


[-- Attachment #2: patch2.txt --]
[-- Type: text/plain, Size: 524 bytes --]

&& (!flag_trapping_math
    || ((cmp == EQ_EXPR || cmp == NE_EXPR)
        && !tree_expr_maybe_signaling_nan_p (@1)
	&& !tree_expr_maybe_signaling_nan_p (@0)))

case NE:
	if (flag_trapping_math
            && (REAL_VALUE_ISSIGNALING_NAN (*d0)
                || REAL_VALUE_ISSIGANLING_NAN (*d1)))
	  return 0;
	return const_true_rtx;

case EQ:
	if (flag_trapping_math
            && (REAL_VALUE_ISSIGNALING_NAN (*d0)
                || REAL_VALUE_ISSIGANLING_NAN (*d1)))
	  return 0;
	return const0_rtx;

RE: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Roger Sayle]

[-- Attachment #1: Type: text/plain, Size: 2595 bytes --]


Doh! Wrong patch2.txt file.  Sorry.

-----Original Message-----
From: Roger Sayle <roger@nextmovesoftware.com> 
Sent: 27 November 2020 10:52
To: 'Jakub Jelinek' <jakub@redhat.com>; 'Joseph S. Myers'
<joseph@codesourcery.com>
Cc: 'Richard Biener' <rguenther@suse.de>; 'gcc-patches@gcc.gnu.org'
<gcc-patches@gcc.gnu.org>
Subject: RE: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]


Hi Jakub,
Technically, PR97965 doesn't explicitly mention equality/inequality, but
you're right, it makes sense to tackle this missed optimization at the same
time as we fix the wrong-code.

On Thu, Nov 26, 2020, Jakub Jelinek wrote:
>On Thu, Nov 26, 2020 at 01:56:03PM -0000, Roger Sayle wrote:
>> --- a/gcc/match.pd
>> +++ b/gcc/match.pd
>> @@ -3998,7 +3998,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>>      (cmp @0 { build_real (TREE_TYPE (@1), dconst0); }))
>>     /* x != NaN is always true, other ops are always false.  */
>>     (if (REAL_VALUE_ISNAN (TREE_REAL_CST (@1))
>> -	&& ! HONOR_SNANS (@1))
>> +	&& ! flag_trapping_math)
>
>Shouldn't this one stay as is for cmp == EQ_EXPR || cmp == NE_EXPR?
>I mean, == or != only raise exceptions on sNaNs, while other simple
comparisons raise on both sNaNs and qNaNs.

The case to be careful of here is that (x == qNaN) can still raise/trap if x
is a sNaN.
Hence, the full condition should include:

&& (!flag_trapping_math
    || ((cmp == EQ_EXPR || cmp == NE_EXPR)
        && !tree_expr_maybe_signaling_nan_p (@1)
        && !tree_expr_maybe_signaling_nan_p (@0)))

Note: I repeat tree_expr_maybe_signaling_nan_p here for symmetry, though
because we know @1 is a NaN, this could have been written as
!tree_expr_signaling_nan_p (@1).

>> --- a/gcc/simplify-rtx.c
>> +++ b/gcc/simplify-rtx.c
>> @@ -5732,12 +5732,13 @@ simplify_const_relational_operation (enum
rtx_code code,
>>        if (REAL_VALUE_ISNAN (*d0) || REAL_VALUE_ISNAN (*d1))
>>  	switch (code)
>>  	  {
>> +	  case NE:
>> +	    return flag_trapping_math ? 0 : const_true_rtx;
>
> And here too (for NE and would need moving EQ later too.

Yep/Agreed.  These cases become:

case NE:
        if (flag_trapping_math
            && (REAL_VALUE_ISSIGNALING_NAN (*d0)
                || REAL_VALUE_ISSIGANLING_NAN (*d1)))
          return 0;
        return const_true_rtx;

case EQ:
        if (flag_trapping_math
            && (REAL_VALUE_ISSIGNALING_NAN (*d0)
                || REAL_VALUE_ISSIGANLING_NAN (*d1)))
          return 0;
        return const0_rtx;


A revised patch is attached (which I've confirmed compiles but haven't
regression tested).

Many thanks again.

Roger
--


[-- Attachment #2: patch2.txt --]
[-- Type: text/plain, Size: 2737 bytes --]

diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 632a241a964..b76e80c02a3 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -12007,8 +12007,8 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
       strict_overflow_p = false;
       if (code == GE_EXPR
 	  && (integer_zerop (arg1)
-	      || (! HONOR_NANS (arg0)
-		  && real_zerop (arg1)))
+	      || (real_zerop (arg1)
+		  && !tree_expr_maybe_nan_p (arg0)))
 	  && tree_expr_nonnegative_warnv_p (arg0, &strict_overflow_p))
 	{
 	  if (strict_overflow_p)
@@ -12024,7 +12024,10 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
       /* Convert ABS_EXPR<x> < 0 to false.  */
       strict_overflow_p = false;
       if (code == LT_EXPR
-	  && (integer_zerop (arg1) || real_zerop (arg1))
+	  && (integer_zerop (arg1)
+	      || (real_zerop (arg1)
+		  && (!flag_trapping_math
+		      || !tree_expr_maybe_nan_p (arg0))))
 	  && tree_expr_nonnegative_warnv_p (arg0, &strict_overflow_p))
 	{
 	  if (strict_overflow_p)
diff --git a/gcc/match.pd b/gcc/match.pd
index b6dfc24af88..ef1bfb42d12 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -4037,7 +4037,10 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
     (cmp @0 { build_real (TREE_TYPE (@1), dconst0); }))
    /* x != NaN is always true, other ops are always false.  */
    (if (REAL_VALUE_ISNAN (TREE_REAL_CST (@1))
-	&& ! HONOR_SNANS (@1))
+	&& (!flag_trapping_math
+	    || ((cmp == EQ_EXPR || cmp == NE_EXPR)
+		&& !tree_expr_maybe_signaling_nan_p (@1)
+		&& !tree_expr_maybe_signaling_nan_p (@0))))
     { constant_boolean_node (cmp == NE_EXPR, type); })
    /* Fold comparisons against infinity.  */
    (if (REAL_VALUE_ISINF (TREE_REAL_CST (@1))
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 47e7aebda8a..dd97a353bff 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -5732,20 +5732,31 @@ simplify_const_relational_operation (enum rtx_code code,
       if (REAL_VALUE_ISNAN (*d0) || REAL_VALUE_ISNAN (*d1))
 	switch (code)
 	  {
+	  case NE:
+	    if (flag_trapping_math
+		&& (REAL_VALUE_ISSIGNALING_NAN (*d0)
+		    || REAL_VALUE_ISSIGNALING_NAN (*d1)))
+	      return 0;
+	    return const_true_rtx;
 	  case UNEQ:
 	  case UNLT:
 	  case UNGT:
 	  case UNLE:
 	  case UNGE:
-	  case NE:
 	  case UNORDERED:
 	    return const_true_rtx;
 	  case EQ:
+	    if (flag_trapping_math
+		&& (REAL_VALUE_ISSIGNALING_NAN (*d0)
+		    || REAL_VALUE_ISSIGNALING_NAN (*d1)))
+	      return 0;
+	    return const0_rtx;
 	  case LT:
 	  case GT:
 	  case LE:
 	  case GE:
 	  case LTGT:
+	    return flag_trapping_math ? 0 : const0_rtx;
 	  case ORDERED:
 	    return const0_rtx;
 	  default:

RE: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Joseph Myers]

On Thu, 26 Nov 2020, Roger Sayle wrote:

> NaNs have a sign-bit, so copyexpr (and negate and ABS_EXPR) are 
> well-defined, and it's reasonable for nonnegative_p to reflect this.  
> IMHO, the true bug is that we can't fold away (any) comparisons against 
> NaN when flag_trapping_math, irrespective of qNaN, -qNaN, sNaN or -sNaN.

I think the comment documenting the semantics of any function mentioning 
"nonnegative" needs to say explicitly which of at least three possible 
things it means, and then callers should be checked to see if that matches 
the semantics they want.

1. !(argument < 0)

2. argument >= 0

3. !signbit(argument)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] fold-const: Don't consider NaN non-negative [PR97965]

[Joseph Myers]

On Thu, 26 Nov 2020, Richard Biener wrote:

> Is copysign (x, NaN) supposed to be well-defined?  We'd stop folding

copysign with NaN arguments (including sNaN) is well-defined and copies 
the sign bit without raising any exceptions.

> this then, no?  I think the ABS_EXPR<x> < 0 to false folding is
> simply incomplete and should first check whether the operands are
> ordered?  That said, NaN is nonnegative but NaN < 0 isn't false(?)

Comparisons involving NaN with < <= > >= == all return false.

For example, with -fno-trapping-math, it's valid to fold fabs (anything) < 
0 to false (preserving any side effects from evaluating "anything"), but 
it's not valid to fold fabs (anything) >= 0 to true, because NaN < 0 and 
NaN >= 0 are both false.  With -ftrapping-math, neither can be folded if 
the argument might be a NaN.

-- 
Joseph S. Myers
joseph@codesourcery.com