From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <aldyh@redhat.com>
Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124])
by sourceware.org (Postfix) with ESMTPS id 9050C3857C4D
for <gcc-patches@gcc.gnu.org>; Sun, 18 Sep 2022 07:10:42 +0000 (GMT)
DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9050C3857C4D
Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com
Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
s=mimecast20190719; t=1663485042;
h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
to:to:cc:mime-version:mime-version:content-type:content-type:
in-reply-to:in-reply-to:references:references;
bh=vLMQteftcgiMdPFZgVqPjgGuqtBQXo9CBAy1Nw9kxss=;
b=MviQ80yVxM9sGQ24mSrUfXUC9VZ/5IGlAe8Xt2YwnkGviaK1H7Z0WGpkF4fML/x3kGZeeL
qQpekcDjo0EwpdAN+Zk8z6AvR2L+JczWeXD3T1kDU5YbRCUgn3D24ATTUiN0spNKLkX5GI
bQ6SFA2EUBye3P/MyYIfd2cyD2j3HHU=
Received: from mail-oo1-f72.google.com (mail-oo1-f72.google.com
[209.85.161.72]) by relay.mimecast.com with ESMTP with STARTTLS
(version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id
us-mta-625-h8qtoe6sN3OAWCR0sz70qg-1; Sun, 18 Sep 2022 03:10:38 -0400
X-MC-Unique: h8qtoe6sN3OAWCR0sz70qg-1
Received: by mail-oo1-f72.google.com with SMTP id n6-20020a4a6106000000b0044b2434319eso13088007ooc.3
for <gcc-patches@gcc.gnu.org>; Sun, 18 Sep 2022 00:10:38 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20210112;
h=to:subject:message-id:date:from:in-reply-to:references:mime-version
:x-gm-message-state:from:to:cc:subject:date;
bh=vLMQteftcgiMdPFZgVqPjgGuqtBQXo9CBAy1Nw9kxss=;
b=tIKd4MTwJwi71eXpMstxEkWTzg+twqBSB9Fa8X1GFjIeAek6anSSes+unFpuFFKHHw
oHWyjV6Z/9yNTLwmNG57l38RAnS7QmWugFkSnYhIsMlilkfmBT6J66gKGKJrUlW4LJjD
6gWt8FH7HRfK/P2fwJhY2xiw6fobREW17IZjAXI08TfNpVTmEic9DpViKeoPLal8udDs
520CUfsoVwiq+IP9fjUhBosjgTTVryzRmytNGokuSNQPljrLKQPtl3Moddb+NPvsfemU
3fE2reWLvfbJ2akHdLFdCDgcBqz9mTavtxE+zNP97eKeKgS4km2WtJyaBRTPzfCXchx4
gtMQ==
X-Gm-Message-State: ACrzQf2Akdv7s+CWkO7wCza0PRay25PlzWqQWwBD0yT0BUUE6JVYNMsf
essJgLkJFDvDx735knfWVwIyTHu3vCnwWga+DP3d50pqIIL/Q4TDPer5K6NDwR2GCd+ufIQoGT1
8f0xpz7q7TnJqPnks1oGRyoBvc9rd21+3cA==
X-Received: by 2002:a05:6870:898e:b0:12b:3e64:e86d with SMTP id f14-20020a056870898e00b0012b3e64e86dmr7076707oaq.265.1663485037671;
Sun, 18 Sep 2022 00:10:37 -0700 (PDT)
X-Google-Smtp-Source: AMsMyM41tmz5cX7zd1rDu61vKXkAfVT9/mojf4kqk3FNUXL3tdsNFRLB6A09YVitimX4xUtKOKy3eB+kqYpW3tgkxVE=
X-Received: by 2002:a05:6870:898e:b0:12b:3e64:e86d with SMTP id
f14-20020a056870898e00b0012b3e64e86dmr7076697oaq.265.1663485037316; Sun, 18
Sep 2022 00:10:37 -0700 (PDT)
MIME-Version: 1.0
References: <20220915054026.1359564-1-aldyh@redhat.com> <CAFiYyc391KNNmzFSV6UJPANfc78Te7nTR6MGDqWkDmAzEiW5Yw@mail.gmail.com>
<CAGm3qMU36AwtR+OggvbhVCxT0zhJh=_GbbggW5wzj-bpy_mzLQ@mail.gmail.com>
<mpt1qsb4t34.fsf@arm.com> <CAGm3qMXtoe4N1nh=Td4sUMX2xA=7Y2i4HT+2QjrSuf=j1yXE+w@mail.gmail.com>
In-Reply-To: <CAGm3qMXtoe4N1nh=Td4sUMX2xA=7Y2i4HT+2QjrSuf=j1yXE+w@mail.gmail.com>
From: Aldy Hernandez <aldyh@redhat.com>
Date: Sun, 18 Sep 2022 09:10:26 +0200
Message-ID: <CAGm3qMWHyQO92kLSbOBqE91C3Qi+ptwXqwxoAChVdvbsuA60bA@mail.gmail.com>
Subject: Re: [PATCH] Rewrite NAN and sign handling in frange
To: Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org>, Richard Biener <richard.guenther@gmail.com>,
Aldy Hernandez <aldyh@redhat.com>, Jakub Jelinek <jakub@redhat.com>, richard.sandiford@arm.com
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Type: text/plain; charset="UTF-8"
X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org
List-Id: <gcc-patches.gcc.gnu.org>
Pushed. We can address any further changes as follow-ups.
Thanks.
Aldy
On Fri, Sep 16, 2022 at 3:26 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> 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