[COMMITTED] Improve union of ranges containing NAN.
Commit Message
Previously [5,6] U NAN would just drop to VARYING. With this patch,
the resulting range becomes [5,6] with the NAN bit set to unknown.
[I still have yet to decide what to do with intersections. ISTM, the
intersection of a known NAN with anything else should be a NAN, but it
could also be undefined (the empty set). I'll have to run some tests
and see. Currently, we drop to VARYING cause well... it's always safe
to give up;-).]
gcc/ChangeLog:
* value-range.cc (early_nan_resolve): Change comment.
(frange::union_): Handle union when one side is a NAN.
(range_tests_nan): Add tests for NAN union.
---
gcc/value-range.cc | 44 ++++++++++++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 10 deletions(-)
Comments
On Tue, Aug 30, 2022 at 5:43 PM Aldy Hernandez via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Previously [5,6] U NAN would just drop to VARYING. With this patch,
> the resulting range becomes [5,6] with the NAN bit set to unknown.
>
> [I still have yet to decide what to do with intersections. ISTM, the
> intersection of a known NAN with anything else should be a NAN, but it
> could also be undefined (the empty set). I'll have to run some tests
> and see. Currently, we drop to VARYING cause well... it's always safe
> to give up;-).]
Giving up in too many places will hide bugs in others for too long ...
> gcc/ChangeLog:
>
> * value-range.cc (early_nan_resolve): Change comment.
> (frange::union_): Handle union when one side is a NAN.
> (range_tests_nan): Add tests for NAN union.
> ---
> gcc/value-range.cc | 44 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> index b6d6c62c06c..473139c6dbd 100644
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -403,7 +403,7 @@ early_nan_resolve (frange &r, const frange &other)
> // There's nothing to do for both NANs.
> if (r.get_nan ().yes_p () == other.get_nan ().yes_p ())
> return false;
> - // But only one NAN complicates things.
> + // ?? Perhaps the intersection of a NAN and anything is a NAN ??.
> r.set_varying (r.type ());
> return true;
> }
> @@ -420,10 +420,22 @@ frange::union_ (const vrange &v)
> *this = r;
> return true;
> }
> - // ?? We could do better here. [5,6] U NAN should be [5,6] with the
> - // NAN maybe bits set. For now, this is conservatively correct.
> - if (get_nan ().yes_p () || r.get_nan ().yes_p ())
> - return early_nan_resolve (*this, r);
> +
> + // If one side has a NAN, the union is just the other side plus the
> + // NAN bit.
> + if (get_nan ().yes_p ())
> + {
> + *this = r;
> + // NOP if NAN already set.
> + set_nan (fp_prop::VARYING);
> + return true;
> + }
> + if (r.get_nan ().yes_p ())
> + {
> + // NOP if NAN already set.
> + set_nan (fp_prop::VARYING);
> + return true;
> + }
>
> bool changed = m_props.union_ (r.m_props);
>
> @@ -3520,6 +3532,7 @@ static void
> range_tests_nan ()
> {
> frange r0, r1;
> + REAL_VALUE_TYPE q, r;
>
> // Equal ranges but with differing NAN bits are not equal.
> r1 = frange_float ("10", "12");
> @@ -3537,13 +3550,24 @@ range_tests_nan ()
> ASSERT_FALSE (r0 == r0);
> ASSERT_TRUE (r0 != r0);
>
> - // Make sure that combining NAN and INF doesn't give any crazy results.
> + // [5,6] U NAN is [5,6] with an unknown NAN bit.
> + r0 = frange_float ("5", "6");
> + r0.set_nan (fp_prop::NO);
> + r1 = frange_nan (float_type_node);
> + r0.union_ (r1);
> + real_from_string (&q, "5");
> + real_from_string (&r, "6");
> + ASSERT_TRUE (real_identical (&q, &r0.lower_bound ()));
> + ASSERT_TRUE (real_identical (&r, &r0.upper_bound ()));
> + ASSERT_TRUE (r0.get_nan ().varying_p ());
> +
> + // NAN U NAN = NAN
> r0 = frange_nan (float_type_node);
> - ASSERT_TRUE (r0.get_nan ().yes_p ());
> - r1 = frange_float ("+Inf", "+Inf");
> + r1 = frange_nan (float_type_node);
> r0.union_ (r1);
> - // [INF, INF] U NAN = VARYING
> - ASSERT_TRUE (r0.varying_p ());
> + ASSERT_TRUE (real_isnan (&r0.lower_bound ()));
> + ASSERT_TRUE (real_isnan (&r1.upper_bound ()));
> + ASSERT_TRUE (r0.get_nan ().yes_p ());
>
> // [INF, INF] ^ NAN = VARYING
> r0 = frange_nan (float_type_node);
> --
> 2.37.1
>
On Wed, Aug 31, 2022 at 8:22 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Tue, Aug 30, 2022 at 5:43 PM Aldy Hernandez via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Previously [5,6] U NAN would just drop to VARYING. With this patch,
> > the resulting range becomes [5,6] with the NAN bit set to unknown.
> >
> > [I still have yet to decide what to do with intersections. ISTM, the
> > intersection of a known NAN with anything else should be a NAN, but it
> > could also be undefined (the empty set). I'll have to run some tests
> > and see. Currently, we drop to VARYING cause well... it's always safe
> > to give up;-).]
>
> Giving up in too many places will hide bugs in others for too long ...
We won't cause bugs, because VARYING is always conservatively correct.
But yes, it's on my list.
I have some queued up patches I'm testing to address some of these
issues. I just wanted to get something correct out and working to get
broader testing.
Aldy
@@ -403,7 +403,7 @@ early_nan_resolve (frange &r, const frange &other)
// There's nothing to do for both NANs.
if (r.get_nan ().yes_p () == other.get_nan ().yes_p ())
return false;
- // But only one NAN complicates things.
+ // ?? Perhaps the intersection of a NAN and anything is a NAN ??.
r.set_varying (r.type ());
return true;
}
@@ -420,10 +420,22 @@ frange::union_ (const vrange &v)
*this = r;
return true;
}
- // ?? We could do better here. [5,6] U NAN should be [5,6] with the
- // NAN maybe bits set. For now, this is conservatively correct.
- if (get_nan ().yes_p () || r.get_nan ().yes_p ())
- return early_nan_resolve (*this, r);
+
+ // If one side has a NAN, the union is just the other side plus the
+ // NAN bit.
+ if (get_nan ().yes_p ())
+ {
+ *this = r;
+ // NOP if NAN already set.
+ set_nan (fp_prop::VARYING);
+ return true;
+ }
+ if (r.get_nan ().yes_p ())
+ {
+ // NOP if NAN already set.
+ set_nan (fp_prop::VARYING);
+ return true;
+ }
bool changed = m_props.union_ (r.m_props);
@@ -3520,6 +3532,7 @@ static void
range_tests_nan ()
{
frange r0, r1;
+ REAL_VALUE_TYPE q, r;
// Equal ranges but with differing NAN bits are not equal.
r1 = frange_float ("10", "12");
@@ -3537,13 +3550,24 @@ range_tests_nan ()
ASSERT_FALSE (r0 == r0);
ASSERT_TRUE (r0 != r0);
- // Make sure that combining NAN and INF doesn't give any crazy results.
+ // [5,6] U NAN is [5,6] with an unknown NAN bit.
+ r0 = frange_float ("5", "6");
+ r0.set_nan (fp_prop::NO);
+ r1 = frange_nan (float_type_node);
+ r0.union_ (r1);
+ real_from_string (&q, "5");
+ real_from_string (&r, "6");
+ ASSERT_TRUE (real_identical (&q, &r0.lower_bound ()));
+ ASSERT_TRUE (real_identical (&r, &r0.upper_bound ()));
+ ASSERT_TRUE (r0.get_nan ().varying_p ());
+
+ // NAN U NAN = NAN
r0 = frange_nan (float_type_node);
- ASSERT_TRUE (r0.get_nan ().yes_p ());
- r1 = frange_float ("+Inf", "+Inf");
+ r1 = frange_nan (float_type_node);
r0.union_ (r1);
- // [INF, INF] U NAN = VARYING
- ASSERT_TRUE (r0.varying_p ());
+ ASSERT_TRUE (real_isnan (&r0.lower_bound ()));
+ ASSERT_TRUE (real_isnan (&r1.upper_bound ()));
+ ASSERT_TRUE (r0.get_nan ().yes_p ());
// [INF, INF] ^ NAN = VARYING
r0 = frange_nan (float_type_node);