[PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Aldy Hernandez]

ISTM that a specifically nonnegative range should not contain -NAN,
otherwise signbit_p() would return false, because we'd be unsure of the
sign.

Do y'all agree?

	PR 68097/tree-optimization

gcc/ChangeLog:

	* value-range.cc (frange::set_nonnegative): Set +NAN.
	(range_tests_signed_zeros): New test.
	* value-range.h (frange::update_nan): New overload to set NAN sign.
---
 gcc/value-range.cc |  9 +++++++++
 gcc/value-range.h  | 14 ++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/gcc/value-range.cc b/gcc/value-range.cc
index 67d5d7fa90f..e432ec8b525 100644
--- a/gcc/value-range.cc
+++ b/gcc/value-range.cc
@@ -752,6 +752,10 @@ void
 frange::set_nonnegative (tree type)
 {
   set (type, dconst0, dconstinf);
+
+  // Set +NAN as the only possibility.
+  if (HONOR_NANS (type))
+    update_nan (/*sign=*/0);
 }
 
 // Here we copy between any two irange's.  The ranges can be legacy or
@@ -3800,6 +3804,11 @@ range_tests_signed_zeros ()
   r1.update_nan ();
   r0.intersect (r1);
   ASSERT_TRUE (r0.known_isnan ());
+
+  r0.set_nonnegative (float_type_node);
+  ASSERT_TRUE (r0.signbit_p (signbit) && !signbit);
+  if (HONOR_NANS (float_type_node))
+    ASSERT_TRUE (r0.maybe_isnan ());
 }
 
 static void
diff --git a/gcc/value-range.h b/gcc/value-range.h
index 3a401f3e4e2..5b261d4f46a 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -312,6 +312,7 @@ public:
   const REAL_VALUE_TYPE &lower_bound () const;
   const REAL_VALUE_TYPE &upper_bound () const;
   void update_nan ();
+  void update_nan (bool sign);
   void clear_nan ();
 
   // fpclassify like API
@@ -1098,6 +1099,19 @@ frange::update_nan ()
     verify_range ();
 }
 
+// Like above, but set the sign of the NAN.
+
+inline void
+frange::update_nan (bool sign)
+{
+  gcc_checking_assert (!undefined_p ());
+  m_pos_nan = !sign;
+  m_neg_nan = sign;
+  normalize_kind ();
+  if (flag_checking)
+    verify_range ();
+}
+
 // Clear the NAN bit and adjust the range.
 
 inline void
-- 
2.37.1

Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Richard Biener]

On Mon, Sep 19, 2022 at 9:59 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> ISTM that a specifically nonnegative range should not contain -NAN,
> otherwise signbit_p() would return false, because we'd be unsure of the
> sign.
>
> Do y'all agree?

what tree_expr_nonnegative_p actually means isn't 100% clear.  For REAL_CST
it actually looks at the sign-bit but we have

(simplify
 /* copysign(x,y) -> fabs(x) if y is nonnegative.  */
 (COPYSIGN_ALL @0 tree_expr_nonnegative_p@1)
 (abs @0))

is abs (@0) OK for sNaNs and -NaN/+NaN?

And we have

/* Convert abs[u] (X)  where X is nonnegative -> (X).  */
(simplify
 (abs tree_expr_nonnegative_p@0)
 @0)

where at least sNaN -> qNaN would be dropped?

And of course

(simplify
 /* signbit(x) -> 0 if x is nonnegative.  */
 (SIGNBIT tree_expr_nonnegative_p@0)
 { integer_zero_node; })

that is, is tree_expr_nonnegative_p actually tree_expr_sign or
does tree_expr_nonnegative (x) mean x >= (typeof(X)) 0
or !(x < (typeof(X))0)?

That said, 'set_nonnegative' could be interpreted to be without
NaNs?

Richard.

>         PR 68097/tree-optimization
>
> gcc/ChangeLog:
>
>         * value-range.cc (frange::set_nonnegative): Set +NAN.
>         (range_tests_signed_zeros): New test.
>         * value-range.h (frange::update_nan): New overload to set NAN sign.
> ---
>  gcc/value-range.cc |  9 +++++++++
>  gcc/value-range.h  | 14 ++++++++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> index 67d5d7fa90f..e432ec8b525 100644
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -752,6 +752,10 @@ void
>  frange::set_nonnegative (tree type)
>  {
>    set (type, dconst0, dconstinf);
> +
> +  // Set +NAN as the only possibility.
> +  if (HONOR_NANS (type))
> +    update_nan (/*sign=*/0);
>  }
>
>  // Here we copy between any two irange's.  The ranges can be legacy or
> @@ -3800,6 +3804,11 @@ range_tests_signed_zeros ()
>    r1.update_nan ();
>    r0.intersect (r1);
>    ASSERT_TRUE (r0.known_isnan ());
> +
> +  r0.set_nonnegative (float_type_node);
> +  ASSERT_TRUE (r0.signbit_p (signbit) && !signbit);
> +  if (HONOR_NANS (float_type_node))
> +    ASSERT_TRUE (r0.maybe_isnan ());
>  }
>
>  static void
> diff --git a/gcc/value-range.h b/gcc/value-range.h
> index 3a401f3e4e2..5b261d4f46a 100644
> --- a/gcc/value-range.h
> +++ b/gcc/value-range.h
> @@ -312,6 +312,7 @@ public:
>    const REAL_VALUE_TYPE &lower_bound () const;
>    const REAL_VALUE_TYPE &upper_bound () const;
>    void update_nan ();
> +  void update_nan (bool sign);
>    void clear_nan ();
>
>    // fpclassify like API
> @@ -1098,6 +1099,19 @@ frange::update_nan ()
>      verify_range ();
>  }
>
> +// Like above, but set the sign of the NAN.
> +
> +inline void
> +frange::update_nan (bool sign)
> +{
> +  gcc_checking_assert (!undefined_p ());
> +  m_pos_nan = !sign;
> +  m_neg_nan = sign;
> +  normalize_kind ();
> +  if (flag_checking)
> +    verify_range ();
> +}
> +
>  // Clear the NAN bit and adjust the range.
>
>  inline void
> --
> 2.37.1
>

Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Aldy Hernandez]

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

On Mon, Sep 19, 2022 at 10:14 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Sep 19, 2022 at 9:59 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> >
> > ISTM that a specifically nonnegative range should not contain -NAN,
> > otherwise signbit_p() would return false, because we'd be unsure of the
> > sign.
> >
> > Do y'all agree?
>
> what tree_expr_nonnegative_p actually means isn't 100% clear.  For REAL_CST
> it actually looks at the sign-bit but we have
>
> (simplify
>  /* copysign(x,y) -> fabs(x) if y is nonnegative.  */
>  (COPYSIGN_ALL @0 tree_expr_nonnegative_p@1)
>  (abs @0))
>
> is abs (@0) OK for sNaNs and -NaN/+NaN?

At least for real_value's, ABS_EXPR works on NAN's.  There's no
special code dealing with them.  We just clear the sign bit:

real_arithmetic:
    case ABS_EXPR:
      *r = *op0;
      r->sign = 0;
      break;

>
> And we have
>
> /* Convert abs[u] (X)  where X is nonnegative -> (X).  */
> (simplify
>  (abs tree_expr_nonnegative_p@0)
>  @0)
>
> where at least sNaN -> qNaN would be dropped?
>
> And of course
>
> (simplify
>  /* signbit(x) -> 0 if x is nonnegative.  */
>  (SIGNBIT tree_expr_nonnegative_p@0)
>  { integer_zero_node; })
>
> that is, is tree_expr_nonnegative_p actually tree_expr_sign or
> does tree_expr_nonnegative (x) mean x >= (typeof(X)) 0
> or !(x < (typeof(X))0)?

I have no idea, but I'm happy to have frange::set_nonnegative() do
whatever you agree on.

Actually TBH, ranger only uses set_nonnegative for call's, not much else:

  if (range_of_builtin_call (r, call, src))
    ;
  else if (gimple_stmt_nonnegative_warnv_p (call, &strict_overflow_p))
    r.set_nonnegative (type);
  else if (gimple_call_nonnull_result_p (call)
       || gimple_call_nonnull_arg (call))
    r.set_nonzero (type);
  else
    r.set_varying (type);

but I guess it's good we do the right thing for correctness sake, and
if it ever gets used by someone else.

>
> That said, 'set_nonnegative' could be interpreted to be without
> NaNs?

Sounds good to me.  How's this?

Aldy

[-- Attachment #2: 0001-PR68097-Non-negative-frange-s-should-not-contain-NAN.patch --]
[-- Type: text/x-patch, Size: 1058 bytes --]

From 7eeb81a9516663d589e692b278907bb5f3d60ea0 Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <aldyh@redhat.com>
Date: Sun, 18 Sep 2022 16:24:36 +0200
Subject: [PATCH] [PR68097] Non-negative frange's should not contain NANs.

	PR 68097/tree-optimization

gcc/ChangeLog:

	* value-range.cc (frange::set_nonnegative): Clear NAN.
	(range_tests_signed_zeros): New test.
---
 gcc/value-range.cc | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gcc/value-range.cc b/gcc/value-range.cc
index 67d5d7fa90f..6e703c7eb43 100644
--- a/gcc/value-range.cc
+++ b/gcc/value-range.cc
@@ -752,6 +752,7 @@ void
 frange::set_nonnegative (tree type)
 {
   set (type, dconst0, dconstinf);
+  clear_nan ();
 }
 
 // Here we copy between any two irange's.  The ranges can be legacy or
@@ -3800,6 +3801,10 @@ range_tests_signed_zeros ()
   r1.update_nan ();
   r0.intersect (r1);
   ASSERT_TRUE (r0.known_isnan ());
+
+  // Non-negative ranges should not contain NANs.
+  r0.set_nonnegative (float_type_node);
+  ASSERT_TRUE (!r0.maybe_isnan ());
 }
 
 static void
-- 
2.37.1

Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Richard Biener]

On Mon, Sep 19, 2022 at 2:52 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> On Mon, Sep 19, 2022 at 10:14 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Mon, Sep 19, 2022 at 9:59 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> > >
> > > ISTM that a specifically nonnegative range should not contain -NAN,
> > > otherwise signbit_p() would return false, because we'd be unsure of the
> > > sign.
> > >
> > > Do y'all agree?
> >
> > what tree_expr_nonnegative_p actually means isn't 100% clear.  For REAL_CST
> > it actually looks at the sign-bit but we have
> >
> > (simplify
> >  /* copysign(x,y) -> fabs(x) if y is nonnegative.  */
> >  (COPYSIGN_ALL @0 tree_expr_nonnegative_p@1)
> >  (abs @0))
> >
> > is abs (@0) OK for sNaNs and -NaN/+NaN?
>
> At least for real_value's, ABS_EXPR works on NAN's.  There's no
> special code dealing with them.  We just clear the sign bit:
>
> real_arithmetic:
>     case ABS_EXPR:
>       *r = *op0;
>       r->sign = 0;
>       break;
>
> >
> > And we have
> >
> > /* Convert abs[u] (X)  where X is nonnegative -> (X).  */
> > (simplify
> >  (abs tree_expr_nonnegative_p@0)
> >  @0)
> >
> > where at least sNaN -> qNaN would be dropped?
> >
> > And of course
> >
> > (simplify
> >  /* signbit(x) -> 0 if x is nonnegative.  */
> >  (SIGNBIT tree_expr_nonnegative_p@0)
> >  { integer_zero_node; })
> >
> > that is, is tree_expr_nonnegative_p actually tree_expr_sign or
> > does tree_expr_nonnegative (x) mean x >= (typeof(X)) 0
> > or !(x < (typeof(X))0)?
>
> I have no idea, but I'm happy to have frange::set_nonnegative() do
> whatever you agree on.
>
> Actually TBH, ranger only uses set_nonnegative for call's, not much else:
>
>   if (range_of_builtin_call (r, call, src))
>     ;
>   else if (gimple_stmt_nonnegative_warnv_p (call, &strict_overflow_p))
>     r.set_nonnegative (type);
>   else if (gimple_call_nonnull_result_p (call)
>        || gimple_call_nonnull_arg (call))
>     r.set_nonzero (type);
>   else
>     r.set_varying (type);
>
> but I guess it's good we do the right thing for correctness sake, and
> if it ever gets used by someone else.
>
> >
> > That said, 'set_nonnegative' could be interpreted to be without
> > NaNs?
>
> Sounds good to me.  How's this?

Hmm, I merely had lots of questions above so I think to answer them
we at least should document what 'set_nonnegative' implies in the
abstract vrange class.

It's probably safer to keep NaN in.  For example unfolded copysign (x, 1.)
will return true for x == -NaN but the result will be a NaN.

Richard.

>
> Aldy

Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Michael Matz]

Hello,

On Mon, 19 Sep 2022, Richard Biener via Gcc-patches wrote:

> > but I guess it's good we do the right thing for correctness sake, and
> > if it ever gets used by someone else.
> >
> > >
> > > That said, 'set_nonnegative' could be interpreted to be without
> > > NaNs?
> >
> > Sounds good to me.  How's this?
> 
> Hmm, I merely had lots of questions above so I think to answer them
> we at least should document what 'set_nonnegative' implies in the
> abstract vrange class.
> 
> It's probably safer to keep NaN in.  For example unfolded copysign (x, 1.)
> will return true for x == -NaN but the result will be a NaN.

FWIW, in IEEE, 'abs' (like 'copy, 'copysign' and 'negate') are not 
arithmetic, they are quiet-computational.  Hence they don't rise 
anything, not even for sNaNs; they copy the input bits and appropriately 
modify the bit pattern according to the specification (i.e. fiddle the 
sign bit).

That also means that a predicate like negative_p(x) that would be 
implemented ala

  copysign(1.0, x) < 0.0

deal with NaNs just fine and is required to correctly capture the sign of 
'x'.  If frange::set_nonnegative is supposed to be used in such contexts 
(and I think it's a good idea if that were the case), then set_nonnegative 
does _not_ imply no-NaN.

In particular I would assume that, given an VAYRING frange FR, that 
FR.set_nonnegative() would result in an frange {[+0.0,+inf],+nan} .


Ciao,
Michael.

Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Aldy Hernandez]

On Mon, Sep 19, 2022 at 4:04 PM Michael Matz <matz@suse.de> wrote:
>
> Hello,
>
> On Mon, 19 Sep 2022, Richard Biener via Gcc-patches wrote:
>
> > > but I guess it's good we do the right thing for correctness sake, and
> > > if it ever gets used by someone else.
> > >
> > > >
> > > > That said, 'set_nonnegative' could be interpreted to be without
> > > > NaNs?
> > >
> > > Sounds good to me.  How's this?
> >
> > Hmm, I merely had lots of questions above so I think to answer them
> > we at least should document what 'set_nonnegative' implies in the
> > abstract vrange class.
> >
> > It's probably safer to keep NaN in.  For example unfolded copysign (x, 1.)
> > will return true for x == -NaN but the result will be a NaN.
>
> FWIW, in IEEE, 'abs' (like 'copy, 'copysign' and 'negate') are not
> arithmetic, they are quiet-computational.  Hence they don't rise
> anything, not even for sNaNs; they copy the input bits and appropriately
> modify the bit pattern according to the specification (i.e. fiddle the
> sign bit).
>
> That also means that a predicate like negative_p(x) that would be
> implemented ala
>
>   copysign(1.0, x) < 0.0

I suppose this means -0.0 is not considered negative, though it has
the signbit set?  FWIW, on real_value's real_isneg() returns true for
-0.0 because it only looks at the sign.

>
> deal with NaNs just fine and is required to correctly capture the sign of
> 'x'.  If frange::set_nonnegative is supposed to be used in such contexts
> (and I think it's a good idea if that were the case), then set_nonnegative
> does _not_ imply no-NaN.
>
> In particular I would assume that, given an VAYRING frange FR, that
> FR.set_nonnegative() would result in an frange {[+0.0,+inf],+nan} .

That was my understanding as well, and what my original patch did.
But again, I'm just the messenger.

Aldy

Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Michael Matz]

Hello,

On Tue, 20 Sep 2022, Aldy Hernandez wrote:

> > FWIW, in IEEE, 'abs' (like 'copy, 'copysign' and 'negate') are not
> > arithmetic, they are quiet-computational.  Hence they don't rise
> > anything, not even for sNaNs; they copy the input bits and appropriately
> > modify the bit pattern according to the specification (i.e. fiddle the
> > sign bit).
> >
> > That also means that a predicate like negative_p(x) that would be
> > implemented ala
> >
> >   copysign(1.0, x) < 0.0
> 
> I suppose this means -0.0 is not considered negative,

It would be considered negative if the predicate is implemented like 
above:
   copysign(1.0, -0.0) == -1.0

But really, that depends on what _our_ definition of negative_p is 
supposed to be.  I think the most reasonable definition is indeed similar 
to above, which in turn is equivalent to simply looking at the sign bit 
(which is what copysign() does), i.e. ...

> though it has
> the signbit set?  FWIW, on real_value's real_isneg() returns true for
> -0.0 because it only looks at the sign.

... this seems the sensible thing.  I just wanted to argue the case that 
set_negative (or the like) which "sets" the sign bit does not make the 
nan-ness go away.  They are orthogonal.

> > deal with NaNs just fine and is required to correctly capture the sign of
> > 'x'.  If frange::set_nonnegative is supposed to be used in such contexts
> > (and I think it's a good idea if that were the case), then set_nonnegative
> > does _not_ imply no-NaN.
> >
> > In particular I would assume that, given an VAYRING frange FR, that
> > FR.set_nonnegative() would result in an frange {[+0.0,+inf],+nan} .
> 
> That was my understanding as well, and what my original patch did.
> But again, I'm just the messenger.

Ah, I obviously haven't followed the thread carefully then.  If that's 
what it was doing then IMO it was the right thing.


Ciao,
Michael.

Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Aldy Hernandez]

On Tue, Sep 20, 2022 at 2:51 PM Michael Matz <matz@suse.de> wrote:
>
> Hello,
>
> On Tue, 20 Sep 2022, Aldy Hernandez wrote:
>
> > > FWIW, in IEEE, 'abs' (like 'copy, 'copysign' and 'negate') are not
> > > arithmetic, they are quiet-computational.  Hence they don't rise
> > > anything, not even for sNaNs; they copy the input bits and appropriately
> > > modify the bit pattern according to the specification (i.e. fiddle the
> > > sign bit).
> > >
> > > That also means that a predicate like negative_p(x) that would be
> > > implemented ala
> > >
> > >   copysign(1.0, x) < 0.0
> >
> > I suppose this means -0.0 is not considered negative,
>
> It would be considered negative if the predicate is implemented like
> above:
>    copysign(1.0, -0.0) == -1.0
>
> But really, that depends on what _our_ definition of negative_p is
> supposed to be.  I think the most reasonable definition is indeed similar
> to above, which in turn is equivalent to simply looking at the sign bit
> (which is what copysign() does), i.e. ...
>
> > though it has
> > the signbit set?  FWIW, on real_value's real_isneg() returns true for
> > -0.0 because it only looks at the sign.
>
> ... this seems the sensible thing.  I just wanted to argue the case that
> set_negative (or the like) which "sets" the sign bit does not make the
> nan-ness go away.  They are orthogonal.
>
> > > deal with NaNs just fine and is required to correctly capture the sign of
> > > 'x'.  If frange::set_nonnegative is supposed to be used in such contexts
> > > (and I think it's a good idea if that were the case), then set_nonnegative
> > > does _not_ imply no-NaN.
> > >
> > > In particular I would assume that, given an VAYRING frange FR, that
> > > FR.set_nonnegative() would result in an frange {[+0.0,+inf],+nan} .
> >
> > That was my understanding as well, and what my original patch did.
> > But again, I'm just the messenger.
>
> Ah, I obviously haven't followed the thread carefully then.  If that's
> what it was doing then IMO it was the right thing.

This brings me back to my original patch :).

Richard, do you agree nonnegative should be [0.0, +INF] U +NAN.

Thanks.
Aldy

Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Jakub Jelinek]

On Tue, Sep 20, 2022 at 04:58:38PM +0200, Aldy Hernandez wrote:
> > > > deal with NaNs just fine and is required to correctly capture the sign of
> > > > 'x'.  If frange::set_nonnegative is supposed to be used in such contexts
> > > > (and I think it's a good idea if that were the case), then set_nonnegative
> > > > does _not_ imply no-NaN.
> > > >
> > > > In particular I would assume that, given an VAYRING frange FR, that
> > > > FR.set_nonnegative() would result in an frange {[+0.0,+inf],+nan} .
> > >
> > > That was my understanding as well, and what my original patch did.
> > > But again, I'm just the messenger.
> >
> > Ah, I obviously haven't followed the thread carefully then.  If that's
> > what it was doing then IMO it was the right thing.
> 
> This brings me back to my original patch :).
> 
> Richard, do you agree nonnegative should be [0.0, +INF] U +NAN.

I agree with that.  And similarly if there is negative that does the
opposite [-INF, -0.0] U -NAN.
Though, in most other places when we see that something may be a NaN, I
think we need to set both +NAN and -NAN, because at least the 2008 version
of IEEE 754 says:

"When either an input or result is NaN, this standard does not interpret the sign of a NaN. Note, however,
that operations on bit strings — copy, negate, abs, copySign — specify the sign bit of a NaN result,
sometimes based upon the sign bit of a NaN operand. The logical predicate totalOrder is also affected by
the sign bit of a NaN operand. For all other operations, this standard does not specify the sign bit of a NaN
result, even when there is only one input NaN, or when the NaN is produced from an invalid
operation."

So not sure if we should count on what NaN sign bit we get normally and what
we get for canonical NaN.  If we could rely on it, then the rule is
that if at least one input to binary operation is NaN, then that NaN is
copied to result, but if both are NaNs, which one is picked isn't specified,
so we might need just union the +NAN and -NAN bits from the operands.
But there are still sNaNs and those ought to be turned into some qNaN and
dunno if that can change the NaN bit (say turn the sNaN into canonical
qNaN).
If neither operand is NaN, but result is NaN because of invalid operation
(0/0, inf-inf, inf+-inf, sqrt (-1) and the like),
the result is qNaN, but dunno if we can rely that it will be one with
positive sign.

	Jakub

Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Aldy Hernandez]

On Tue, Sep 20, 2022 at 5:10 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Sep 20, 2022 at 04:58:38PM +0200, Aldy Hernandez wrote:
> > > > > deal with NaNs just fine and is required to correctly capture the sign of
> > > > > 'x'.  If frange::set_nonnegative is supposed to be used in such contexts
> > > > > (and I think it's a good idea if that were the case), then set_nonnegative
> > > > > does _not_ imply no-NaN.
> > > > >
> > > > > In particular I would assume that, given an VAYRING frange FR, that
> > > > > FR.set_nonnegative() would result in an frange {[+0.0,+inf],+nan} .
> > > >
> > > > That was my understanding as well, and what my original patch did.
> > > > But again, I'm just the messenger.
> > >
> > > Ah, I obviously haven't followed the thread carefully then.  If that's
> > > what it was doing then IMO it was the right thing.
> >
> > This brings me back to my original patch :).
> >
> > Richard, do you agree nonnegative should be [0.0, +INF] U +NAN.
>
> I agree with that.  And similarly if there is negative that does the
> opposite [-INF, -0.0] U -NAN.
> Though, in most other places when we see that something may be a NaN, I
> think we need to set both +NAN and -NAN, because at least the 2008 version
> of IEEE 754 says:

Yeah, every other place does update_nan() with no arguments which sets
+-NAN.  The only use of update_nan(bool signbit) is this patch.

>
> "When either an input or result is NaN, this standard does not interpret the sign of a NaN. Note, however,
> that operations on bit strings — copy, negate, abs, copySign — specify the sign bit of a NaN result,
> sometimes based upon the sign bit of a NaN operand. The logical predicate totalOrder is also affected by
> the sign bit of a NaN operand. For all other operations, this standard does not specify the sign bit of a NaN
> result, even when there is only one input NaN, or when the NaN is produced from an invalid
> operation."

Ughh, that means that my upcoming PLUS_EXPR implementation will have
to keep better track of NAN signs.

Pushed original patch.

Thanks.
Aldy

>
> So not sure if we should count on what NaN sign bit we get normally and what
> we get for canonical NaN.  If we could rely on it, then the rule is
> that if at least one input to binary operation is NaN, then that NaN is
> copied to result, but if both are NaNs, which one is picked isn't specified,
> so we might need just union the +NAN and -NAN bits from the operands.
> But there are still sNaNs and those ought to be turned into some qNaN and
> dunno if that can change the NaN bit (say turn the sNaN into canonical
> qNaN).
> If neither operand is NaN, but result is NaN because of invalid operation
> (0/0, inf-inf, inf+-inf, sqrt (-1) and the like),
> the result is qNaN, but dunno if we can rely that it will be one with
> positive sign.
>
>         Jakub
>

Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.

[Aldy Hernandez]

On Mon, Sep 19, 2022 at 3:42 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Sep 19, 2022 at 2:52 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >
> > On Mon, Sep 19, 2022 at 10:14 AM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > >
> > > On Mon, Sep 19, 2022 at 9:59 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> > > >
> > > > ISTM that a specifically nonnegative range should not contain -NAN,
> > > > otherwise signbit_p() would return false, because we'd be unsure of the
> > > > sign.
> > > >
> > > > Do y'all agree?
> > >
> > > what tree_expr_nonnegative_p actually means isn't 100% clear.  For REAL_CST
> > > it actually looks at the sign-bit but we have
> > >
> > > (simplify
> > >  /* copysign(x,y) -> fabs(x) if y is nonnegative.  */
> > >  (COPYSIGN_ALL @0 tree_expr_nonnegative_p@1)
> > >  (abs @0))
> > >
> > > is abs (@0) OK for sNaNs and -NaN/+NaN?
> >
> > At least for real_value's, ABS_EXPR works on NAN's.  There's no
> > special code dealing with them.  We just clear the sign bit:
> >
> > real_arithmetic:
> >     case ABS_EXPR:
> >       *r = *op0;
> >       r->sign = 0;
> >       break;
> >
> > >
> > > And we have
> > >
> > > /* Convert abs[u] (X)  where X is nonnegative -> (X).  */
> > > (simplify
> > >  (abs tree_expr_nonnegative_p@0)
> > >  @0)
> > >
> > > where at least sNaN -> qNaN would be dropped?
> > >
> > > And of course
> > >
> > > (simplify
> > >  /* signbit(x) -> 0 if x is nonnegative.  */
> > >  (SIGNBIT tree_expr_nonnegative_p@0)
> > >  { integer_zero_node; })
> > >
> > > that is, is tree_expr_nonnegative_p actually tree_expr_sign or
> > > does tree_expr_nonnegative (x) mean x >= (typeof(X)) 0
> > > or !(x < (typeof(X))0)?
> >
> > I have no idea, but I'm happy to have frange::set_nonnegative() do
> > whatever you agree on.
> >
> > Actually TBH, ranger only uses set_nonnegative for call's, not much else:
> >
> >   if (range_of_builtin_call (r, call, src))
> >     ;
> >   else if (gimple_stmt_nonnegative_warnv_p (call, &strict_overflow_p))
> >     r.set_nonnegative (type);
> >   else if (gimple_call_nonnull_result_p (call)
> >        || gimple_call_nonnull_arg (call))
> >     r.set_nonzero (type);
> >   else
> >     r.set_varying (type);
> >
> > but I guess it's good we do the right thing for correctness sake, and
> > if it ever gets used by someone else.
> >
> > >
> > > That said, 'set_nonnegative' could be interpreted to be without
> > > NaNs?
> >
> > Sounds good to me.  How's this?
>
> Hmm, I merely had lots of questions above so I think to answer them
> we at least should document what 'set_nonnegative' implies in the
> abstract vrange class.
>
> It's probably safer to keep NaN in.  For example unfolded copysign (x, 1.)
> will return true for x == -NaN but the result will be a NaN.

Come to think of it, perhaps having set_nonnegative in the API is
overkill.  It's either nonsensical or awkward for say pointers or
strings.  There is only one use of it in all of ranger.  It's only
used to set a range for the result from gimple_stmt_nonnegative* on an
unknown call:

 else if (gimple_stmt_nonnegative_warnv_p (call, &strict_overflow_p))
    r.set_nonnegative (type);

I would just remove the API entry point and push whatever we decide
onto the actual use.  No point confusing everyone at large.

So... what does gimple_stmt_nonnegative_warnv_p imply for floats?  My
gut feeling is [+0.0,+INF] U +NAN, but I'm happy to do whatever y'all
agree on.

Aldy