From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 22A833899424 for ; Fri, 16 Sep 2022 08:33:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 22A833899424 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 29C121D34; Fri, 16 Sep 2022 01:33:12 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.62]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 040683F73D; Fri, 16 Sep 2022 01:33:04 -0700 (PDT) From: Richard Sandiford To: Aldy Hernandez via Gcc-patches Mail-Followup-To: Aldy Hernandez via Gcc-patches ,Richard Biener , Aldy Hernandez , Jakub Jelinek , richard.sandiford@arm.com Cc: Richard Biener , Aldy Hernandez , Jakub Jelinek Subject: Re: [PATCH] Rewrite NAN and sign handling in frange References: <20220915054026.1359564-1-aldyh@redhat.com> Date: Fri, 16 Sep 2022 09:33:03 +0100 In-Reply-To: (Aldy Hernandez via Gcc-patches's message of "Thu, 15 Sep 2022 22:44:45 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-41.9 required=5.0 tests=BAYES_00,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Aldy Hernandez via Gcc-patches writes: > On Thu, Sep 15, 2022 at 9:06 AM Richard Biener > wrote: >> >> On Thu, Sep 15, 2022 at 7:41 AM Aldy Hernandez 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