[PATCH] Rewrite NAN and sign handling in frange

Aldy Hernandez aldyh@redhat.com
Fri Sep 16 13:26:13 GMT 2022


On Fri, Sep 16, 2022 at 10:33 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> 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.

Ughh, fair enough.  I've settled on:

  bool known_isfinite () const;
  bool known_isnan () const;
  bool known_isinf () const;
  bool maybe_isnan () const;
  bool maybe_isinf () const;
  bool signbit_p (bool &signbit) const;

Let me know what you think.

In this next revision, I have addressed a few of the suggestions by
Richi, though I have left the out-of-line handling of NANs for
intersect/union because I still find them more readable (for now).
This may get shuffled again if we implement frange_base and frange, so
hang on.

Also, I opted to implement ][ NAN with m_kind == VR_NAN instead of
overloading VR_UNDEFINED.  This makes the code less confusing.

I have retested on x86-64 Linux.  Let me know what y'all think.

Thanks.
Aldy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Rewrite-NAN-and-sign-handling-in-frange.patch
Type: text/x-patch
Size: 49883 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20220916/1d3742fe/attachment-0001.bin>


More information about the Gcc-patches mailing list