From: Richard Sandiford <richard.sandiford@arm.com>
To: Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org>
Cc: Richard Biener <richard.guenther@gmail.com>,
Aldy Hernandez <aldyh@redhat.com>,
Jakub Jelinek <jakub@redhat.com>
Subject: Re: [PATCH] Rewrite NAN and sign handling in frange
Date: Fri, 16 Sep 2022 09:33:03 +0100 [thread overview]
Message-ID: <mpt1qsb4t34.fsf@arm.com> (raw)
In-Reply-To: <CAGm3qMU36AwtR+OggvbhVCxT0zhJh=_GbbggW5wzj-bpy_mzLQ@mail.gmail.com> (Aldy Hernandez via Gcc-patches's message of "Thu, 15 Sep 2022 22:44:45 +0200")
Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Thu, Sep 15, 2022 at 9:06 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
>>
>> On Thu, Sep 15, 2022 at 7:41 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>> >
>> > Hi Richard. Hi all.
>> >
>> > The attatched patch rewrites the NAN and sign handling, dropping both
>> > tristates in favor of a pair of boolean flags for NANs, and nothing at
>> > all for signs. The signs are tracked in the range itself, so now it's
>> > possible to describe things like [-0.0, +0.0] +NAN, [+0, +0], [-5, +0],
>> > [+0, 3] -NAN, etc.
>> >
>> > There are a lot of changes, as the tristate was quite pervasive. I
>> > could use another pair of eyes. The code IMO is cleaner and handles
>> > all the cases we discussed.
>> >
>> > Here is an example of the various ranges and how they are displayed:
>> >
>> > [frange] float VARYING NAN ;; Varying includes NAN
>> > [frange] UNDEFINED ;; Empty set as always
>> > [frange] float [] NAN ;; Unknown sign NAN
>> > [frange] float [] -NAN ;; -NAN
>> > [frange] float [] +NAN ;; +NAN
>> > [frange] float [-0.0, 0.0] ;; All zeros.
>> > [frange] float [-0.0, -0.0] NAN ;; -0 or NAN.
>> > [frange] float [-5.0e+0, -1.0e+0] +NAN ;; [-5, -1] or +NAN
>> > [frange] float [-5.0e+0, -0.0] NAN ;; [-5, -0] or +-NAN
>> > [frange] float [-5.0e+0, -0.0] ;; [-5, -0]
>> > [frange] float [5.0e+0, 1.0e+1] ;; [5, 10]
>> >
>> > We could represent an unknown sign with +NAN -NAN if preferred.
>>
>> maybe -+NAN or +-NAN? I prefer to somehow show both signs for clarity
>
> Sure.
>
>>
>> >
>> > Notice the NAN signs are decoupled from the range, so we can represent
>> > a negative range with a positive NAN. For this range,
>> > frange::known_bit() would return false, as only when the signs of the
>> > NANs and range agree can we be certain.
>> >
>> > There is no longer any pessimization of ranges for intersects
>> > involving NANs. Also, union and intersect work with signed zeros:
>> >
>> > // [-0, x] U [+0, x] => [-0, x]
>> > // [ x, -0] U [ x, +0] => [ x, +0]
>> > // [-0, x] ^ [+0, x] => [+0, x]
>> > // [ x, -0] ^ [ x, +0] => [ x, -0]
>> >
>> > The special casing for signed zeros in the singleton code is gone in
>> > favor of just making sure the signs in the range agree, that is
>> > [-0, -0] for example.
>> >
>> > I have removed the idea that a known NAN is a "range", so a NAN is no
>> > longer in the endpoints itself. Requesting the bound of a known NAN
>> > is a hard fail. For that matter, we don't store the actual NAN in the
>> > range. The only information we have are the set of boolean flags.
>> > This way we make sure nothing seeps into the frange. This also means
>> > it's explicit that we don't track anything but the sign in NANs. We
>> > can revisit this if we desire to track signalling or whatever
>> > concoction y'all can imagine.
>> >
>> > All in all, I'm quite happy with this. It does look better, and we
>> > handle all the corner cases we couldn't before. Thanks for the
>> > suggestion.
>> >
>> > Regstrapped with mpfr tests on x86-64 and ppc64le Linux. Selftests
>> > were also run with -ffinite-math-only on x86-64.
>> >
>> > At Jakub's suggestion, I built lapack with associated tests. They
>> > pass on x86-64 and ppc64le Linux with no regressions from mainline.
>> > As a sanity check, I also ran them for -ffinite-math-only on x86 which
>> > (as expected) returned:
>> >
>> > NaN arithmetic did not perform per the ieee spec
>> >
>> > Otherwise, all tests pass for -ffinite-math-only.
>> >
>> > How does this look?
>>
>> Overall it looks good.
>>
>> Reading ::intersect and ::union I find it less clear to spread out the _nan
>> cases into separate functions.
>
> OK, will inline them.
>
>>
>> Can you add a comment to frange that its representation is
>> a single value-range specified by m_type, m_min, m_max
>> unioned with the set of { -NaN, +NaN }? Because somehow
>> the ::undefined_p vs. m_type == VR_UNDEFINED checks are
>> a bit confusing to the occasional reader can we instead use
>> ::nan_p to complement ::undefined_p?
>
> Wouldn't that just make nan_p the same as known_nan? Speaking of
> which, I'm not a big fan of known_nan. Perhaps we should rename all
> the known_foo variants to foo_p variants? Or...maybe even:
>
> // fpclassify like API
> bool isfinite () const;
> bool isinf () const;
> bool maybe_isinf () const;
> bool isnan () const;
> bool maybe_isnan () const;
> bool signbit_p (bool &signbit) const;
>
> That would make it clear how they map to the fpclassify API. And the
> signbit_p() follows what we do for singleton_p(tree *).
>
> isnan() would be your nan_p suggestion.
FWIW, the reason I didn't do this with the poly_int stuff is that
it makes negative conditions harder to reason about. It's easy for
tired eyes to read:
!isfinite()
as meaning "is infinite", especially since there isn't a separate
isinfinite() query. But if isfinite() is equivalent to known_isfinite()
then !isfinite() instead means "might be infinite". !known_isfinite()
IMO makes that more explicit.
Thanks,
Richard
next prev parent reply other threads:[~2022-09-16 8:33 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-15 5:40 Aldy Hernandez
2022-09-15 7:06 ` Richard Biener
2022-09-15 20:44 ` Aldy Hernandez
2022-09-16 8:33 ` Richard Sandiford [this message]
2022-09-16 13:26 ` Aldy Hernandez
2022-09-18 7:10 ` Aldy Hernandez
2022-09-27 13:00 ` Mikael Morin
2022-11-02 13:35 ` 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=mpt1qsb4t34.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=aldyh@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).