From: Aldy Hernandez <aldyh@redhat.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: GCC patches <gcc-patches@gcc.gnu.org>,
Jakub Jelinek <jakub@redhat.com>,
Andrew MacLeod <amacleod@redhat.com>
Subject: Re: [PATCH] [PR68097] frange::set_nonnegative should not contain -NAN.
Date: Mon, 19 Sep 2022 14:51:47 +0200 [thread overview]
Message-ID: <CAGm3qMV7W7w=fLUTpc4NDEASRpXwfboW99qggp7LGuF_rFJ_pg@mail.gmail.com> (raw)
In-Reply-To: <CAFiYyc1DneZGvjLykXKyPjdCLQGD9sMpSi6YB==Sdx8ZqivBJA@mail.gmail.com>
[-- 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
next prev parent reply other threads:[~2022-09-19 12:52 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-19 7:59 Aldy Hernandez
2022-09-19 8:14 ` Richard Biener
2022-09-19 12:51 ` Aldy Hernandez [this message]
2022-09-19 13:42 ` Richard Biener
2022-09-19 13:58 ` Michael Matz
2022-09-20 5:25 ` Aldy Hernandez
2022-09-20 12:51 ` Michael Matz
2022-09-20 14:58 ` Aldy Hernandez
2022-09-20 15:09 ` Jakub Jelinek
2022-09-20 18:08 ` Aldy Hernandez
2022-09-20 5:32 ` Aldy Hernandez
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='CAGm3qMV7W7w=fLUTpc4NDEASRpXwfboW99qggp7LGuF_rFJ_pg@mail.gmail.com' \
--to=aldyh@redhat.com \
--cc=amacleod@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=richard.guenther@gmail.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).