From: Richard Biener <richard.guenther@gmail.com>
To: Aldy Hernandez <aldyh@redhat.com>
Cc: GCC patches <gcc-patches@gcc.gnu.org>, Jakub Jelinek <jakub@redhat.com>
Subject: Re: [PATCH] [PR tree-optimization/107365] Check HONOR_NANS instead of flag_finite_math_only in frange:verify_range.
Date: Mon, 24 Oct 2022 09:34:26 +0200 [thread overview]
Message-ID: <CAFiYyc3mmt4dZPcMjV2cjEjLUtvAOMtL-81Ar82xX4gUO=zrDQ@mail.gmail.com> (raw)
In-Reply-To: <20221023145633.501586-1-aldyh@redhat.com>
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
>
prev parent reply other threads:[~2022-10-24 7:34 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-23 14:56 Aldy Hernandez
2022-10-24 7:34 ` Richard Biener [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc3mmt4dZPcMjV2cjEjLUtvAOMtL-81Ar82xX4gUO=zrDQ@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=aldyh@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).