diff mbox series

[PR,tree-optimization/107365] Check HONOR_NANS instead of flag_finite_math_only in frange:verify_range.

Message ID 20221023145633.501586-1-aldyh@redhat.com
State Accepted
Headers
Series [PR,tree-optimization/107365] Check HONOR_NANS instead of flag_finite_math_only in frange:verify_range. |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Aldy Hernandez Oct. 23, 2022, 2:56 p.m. UTC 
  [Jakub and other FP experts, would this be OK, or am I missing
something?]

Vax does not seem to have !flag_finite_math_only, but float_type_node
does not HONOR_NANS.  The check in frange::verify_range dependend on
flag_finite_math_only, which is technically not correct since
frange::set_varying() checks HONOR_NANS instead of
flag_finite_math_only.

I'm actually getting tired of flag_finite_math_only and
!flag_finite_math_only discrepancies in the selftests (Vax and rx-elf
come to mind).  I think we should just test both alternatives in the
selftests as in this patch.

We could also check flag_finite_math_only=0 with a float_type_node
that does not HONOR_NANs, but I have no idea how to twiddle
FLOAT_MODE_FORMAT temporarily, and that may be over thinking it.

How does this look?

	PR tree-optimization/107365

gcc/ChangeLog:

	* value-range.cc (frange::verify_range): Predicate NAN check in
	VARYING range on HONOR_NANS instead of flag_finite_math_only.
	(range_tests_floats): Same.
	(range_tests_floats_various): New.
	(range_tests): Call range_tests_floats_various.
---
 gcc/value-range.cc | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

Comments

Richard Biener Oct. 24, 2022, 7:34 a.m. UTC | #1 
On Sun, Oct 23, 2022 at 4:57 PM Aldy Hernandez via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> [Jakub and other FP experts, would this be OK, or am I missing
> something?]
>
> Vax does not seem to have !flag_finite_math_only, but float_type_node
> does not HONOR_NANS.  The check in frange::verify_range dependend on
> flag_finite_math_only, which is technically not correct since
> frange::set_varying() checks HONOR_NANS instead of
> flag_finite_math_only.
>
> I'm actually getting tired of flag_finite_math_only and
> !flag_finite_math_only discrepancies in the selftests (Vax and rx-elf
> come to mind).  I think we should just test both alternatives in the
> selftests as in this patch.
>
> We could also check flag_finite_math_only=0 with a float_type_node
> that does not HONOR_NANs, but I have no idea how to twiddle
> FLOAT_MODE_FORMAT temporarily, and that may be over thinking it.
>
> How does this look?

OK.  As said elsewhere checking flag_* is never correct for any of the
FP type features, always use HONOR_*.

Thanks,
Richard.

>         PR tree-optimization/107365
>
> gcc/ChangeLog:
>
>         * value-range.cc (frange::verify_range): Predicate NAN check in
>         VARYING range on HONOR_NANS instead of flag_finite_math_only.
>         (range_tests_floats): Same.
>         (range_tests_floats_various): New.
>         (range_tests): Call range_tests_floats_various.
> ---
>  gcc/value-range.cc | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> index d779e9819e2..d8ee6ec0d0f 100644
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -720,13 +720,13 @@ frange::verify_range ()
>        gcc_checking_assert (!m_type);
>        return;
>      case VR_VARYING:
> -      if (flag_finite_math_only)
> -       gcc_checking_assert (!m_pos_nan && !m_neg_nan);
> -      else
> -       gcc_checking_assert (m_pos_nan && m_neg_nan);
>        gcc_checking_assert (m_type);
>        gcc_checking_assert (frange_val_is_min (m_min, m_type));
>        gcc_checking_assert (frange_val_is_max (m_max, m_type));
> +      if (HONOR_NANS (m_type))
> +       gcc_checking_assert (m_pos_nan && m_neg_nan);
> +      else
> +       gcc_checking_assert (!m_pos_nan && !m_neg_nan);
>        return;
>      case VR_RANGE:
>        gcc_checking_assert (m_type);
> @@ -3957,10 +3957,9 @@ range_tests_floats ()
>    // A range of [-INF,+INF] is actually VARYING if no other properties
>    // are set.
>    r0 = frange_float ("-Inf", "+Inf");
> -  if (r0.maybe_isnan ())
> -    ASSERT_TRUE (r0.varying_p ());
> +  ASSERT_TRUE (r0.varying_p ());
>    // ...unless it has some special property...
> -  if (!flag_finite_math_only)
> +  if (HONOR_NANS (r0.type ()))
>      {
>        r0.clear_nan ();
>        ASSERT_FALSE (r0.varying_p ());
> @@ -4041,6 +4040,24 @@ range_tests_floats ()
>      }
>  }
>
> +// Run floating range tests for various combinations of NAN and INF
> +// support.
> +
> +static void
> +range_tests_floats_various ()
> +{
> +  int save_finite_math_only = flag_finite_math_only;
> +
> +  // Test -ffinite-math-only.
> +  flag_finite_math_only = 1;
> +  range_tests_floats ();
> +  // Test -fno-finite-math-only.
> +  flag_finite_math_only = 0;
> +  range_tests_floats ();
> +
> +  flag_finite_math_only = save_finite_math_only;
> +}
> +
>  void
>  range_tests ()
>  {
> @@ -4049,7 +4066,7 @@ range_tests ()
>    range_tests_int_range_max ();
>    range_tests_strict_enum ();
>    range_tests_nonzero_bits ();
> -  range_tests_floats ();
> +  range_tests_floats_various ();
>    range_tests_misc ();
>  }
>
> --
> 2.37.3
>
diff mbox series

Patch

diff --git a/gcc/value-range.cc b/gcc/value-range.cc
index d779e9819e2..d8ee6ec0d0f 100644
--- a/gcc/value-range.cc
+++ b/gcc/value-range.cc
@@ -720,13 +720,13 @@  frange::verify_range ()
       gcc_checking_assert (!m_type);
       return;
     case VR_VARYING:
-      if (flag_finite_math_only)
-	gcc_checking_assert (!m_pos_nan && !m_neg_nan);
-      else
-	gcc_checking_assert (m_pos_nan && m_neg_nan);
       gcc_checking_assert (m_type);
       gcc_checking_assert (frange_val_is_min (m_min, m_type));
       gcc_checking_assert (frange_val_is_max (m_max, m_type));
+      if (HONOR_NANS (m_type))
+	gcc_checking_assert (m_pos_nan && m_neg_nan);
+      else
+	gcc_checking_assert (!m_pos_nan && !m_neg_nan);
       return;
     case VR_RANGE:
       gcc_checking_assert (m_type);
@@ -3957,10 +3957,9 @@  range_tests_floats ()
   // A range of [-INF,+INF] is actually VARYING if no other properties
   // are set.
   r0 = frange_float ("-Inf", "+Inf");
-  if (r0.maybe_isnan ())
-    ASSERT_TRUE (r0.varying_p ());
+  ASSERT_TRUE (r0.varying_p ());
   // ...unless it has some special property...
-  if (!flag_finite_math_only)
+  if (HONOR_NANS (r0.type ()))
     {
       r0.clear_nan ();
       ASSERT_FALSE (r0.varying_p ());
@@ -4041,6 +4040,24 @@  range_tests_floats ()
     }
 }
 
+// Run floating range tests for various combinations of NAN and INF
+// support.
+
+static void
+range_tests_floats_various ()
+{
+  int save_finite_math_only = flag_finite_math_only;
+
+  // Test -ffinite-math-only.
+  flag_finite_math_only = 1;
+  range_tests_floats ();
+  // Test -fno-finite-math-only.
+  flag_finite_math_only = 0;
+  range_tests_floats ();
+
+  flag_finite_math_only = save_finite_math_only;
+}
+
 void
 range_tests ()
 {
@@ -4049,7 +4066,7 @@  range_tests ()
   range_tests_int_range_max ();
   range_tests_strict_enum ();
   range_tests_nonzero_bits ();
-  range_tests_floats ();
+  range_tests_floats_various ();
   range_tests_misc ();
 }