Pushed. We can address any further changes as follow-ups. Thanks. Aldy
On Fri, Sep 16, 2022 at 3:26 PM Aldy Hernandez <al...@redhat.com> wrote: > > On Fri, Sep 16, 2022 at 10:33 AM Richard Sandiford > <richard.sandif...@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.guent...@gmail.com> wrote: > > >> > > >> On Thu, Sep 15, 2022 at 7:41 AM Aldy Hernandez <al...@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