range-op-float: Fix up -ffinite-math-only range extension and don't extend into infinities [PR109008]
Message ID | ZArlUJOn1HBZ44yJ@tucnak |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 4B8023850202 for <patchwork@sourceware.org>; Fri, 10 Mar 2023 08:08:21 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 4B8023850202 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1678435701; bh=0kD2epsvnVZsyBnD4r4KN9RYaGkClNPcXNEPH10TzSM=; h=Date:To:Cc:Subject:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=KgsEtiDgpM3jEXUYon6RDb+cUZMqtg6yv8tEHupSW2Jj6Tj3op6W62mgbwkA/28G9 WVRETcnXgtX9tX06wwfxzuiP384iNnc8o83u4i0iL9ekzNMf1KwQI+i9Mr/hItgutk 8JQADvPiBEgnp/xyGPoHnj2dXglR1QODqrVgCndg= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 551993858C54 for <gcc-patches@gcc.gnu.org>; Fri, 10 Mar 2023 08:07:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 551993858C54 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-444-HK2B2vA-Pe2aaQyaAcM9ow-1; Fri, 10 Mar 2023 03:07:49 -0500 X-MC-Unique: HK2B2vA-Pe2aaQyaAcM9ow-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id BFE1380D0F0; Fri, 10 Mar 2023 08:07:48 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7AA672026D4B; Fri, 10 Mar 2023 08:07:48 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 32A87jrB844218 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 10 Mar 2023 09:07:46 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 32A87jal844217; Fri, 10 Mar 2023 09:07:45 +0100 Date: Fri, 10 Mar 2023 09:07:44 +0100 To: Richard Biener <rguenther@suse.de>, Aldy Hernandez <aldyh@redhat.com> Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] range-op-float: Fix up -ffinite-math-only range extension and don't extend into infinities [PR109008] Message-ID: <ZArlUJOn1HBZ44yJ@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jakub Jelinek <jakub@redhat.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
range-op-float: Fix up -ffinite-math-only range extension and don't extend into infinities [PR109008]
|
|
Commit Message
Jakub Jelinek
March 10, 2023, 8:07 a.m. UTC
Hi! The following patch does two things (both related to range extension around the boundaries). The first part (in the 2 real_isfinite blocks) is to make the ranges narrower when the old boundaries are minimum and/or maximum representable finite number. In that case frange_nextafter gives -Inf or +Inf, but then the resulting computed reverse range is very far from the actually needed range, usually extends up to infinity or could even result in NaNs. While infinities are really the next representable numbers in the corresponding mode, REAL_VALUE_TYPE is actually a type with wider range for exponent and 160 bit precision, so the patch instead uses nextafter number in a hypothetical floating point format with the same mantissa precision but wider range of exponents. This significantly improves the actual ranges of the reverse operations, while still making them conservatively correct. The second part is a fix for miscompilation of the new testcase below. For -ffinite-math-only, without this patch we extend the minimum and/or maximum representable finite number to -Inf or +Inf, with the patch to some number outside of the normal exponent range of the mode, but then we use set which canonicalizes it and turns the boundaries back to the minimum and/or maximum representable finite numbers, but because in say [__DBL_MAX__, __DBL_MAX__] = op1 + [__DBL_MAX__, __DBL_MAX__] op1 can be larger than 0, up to the largest number which rounds to even down back to __DBL_MAX__ and there are still no infinities involved, it needs to work even with -ffinite-math-only. So, we really need to widen the lhs range a little bit even in that case. The patch does that through temporarily clearing -ffinite-math-only, such that the value with infinities or the outside of bounds values passes the setting and verification (the VR_VARYING case is needed because we get ICEs otherwise, but when lhs is VR_VARYING in -ffast-math, i.e. minimum to maximum representable finite and both signs of NaN, then set does all we need, we don't need to or in a NaN range). We don't really later use the range in a way that would become a problem that it is wider than varying, we actually just perform maths on the two boundaries. As I said in the PR, this doesn't fix the !MODE_HAS_INFINITIES case, I believe we actually need to treat the boundary values as infinities in that case because they (probably) work like that, but it is unclear if it is just the reverse operation lhs widening that is a problem there, or whether it is a general problem. I have zero experience with floating points without infinities (PDP11, some ARM half type?, what else?). Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2023-03-10 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/109008 * range-op-float.cc (float_widen_lhs_range): If lb is minimum representable finite number or ub is maximum representable finite number, instead of widening it to -inf or inf widen it to negative or positive 0x0.8p+(EMAX+1). Temporarily clear flag_finite_math_only when canonicalizing the widened range. * gcc.dg/pr109008.c: New test. Jakub
Comments
On Fri, 10 Mar 2023, Jakub Jelinek wrote: > Hi! > > The following patch does two things (both related to range extension > around the boundaries). > > The first part (in the 2 real_isfinite blocks) is to make the ranges > narrower when the old boundaries are minimum and/or maximum representable > finite number. In that case frange_nextafter gives -Inf or +Inf, > but then the resulting computed reverse range is very far from the actually > needed range, usually extends up to infinity or could even result in NaNs. > While infinities are really the next representable numbers in the > corresponding mode, REAL_VALUE_TYPE is actually a type with wider range > for exponent and 160 bit precision, so the patch instead uses > nextafter number in a hypothetical floating point format with the same > mantissa precision but wider range of exponents. This significantly > improves the actual ranges of the reverse operations, while still making > them conservatively correct. > > The second part is a fix for miscompilation of the new testcase below. > For -ffinite-math-only, without this patch we extend the minimum and/or > maximum representable finite number to -Inf or +Inf, with the patch to > some number outside of the normal exponent range of the mode, but then > we use set which canonicalizes it and turns the boundaries back to > the minimum and/or maximum representable finite numbers, but because > in say [__DBL_MAX__, __DBL_MAX__] = op1 + [__DBL_MAX__, __DBL_MAX__] > op1 can be larger than 0, up to the largest number which rounds to even > down back to __DBL_MAX__ and there are still no infinities involved, > it needs to work even with -ffinite-math-only. So, we really need to > widen the lhs range a little bit even in that case. The patch does > that through temporarily clearing -ffinite-math-only, such that the > value with infinities or the outside of bounds values passes the > setting and verification (the VR_VARYING case is needed because > we get ICEs otherwise, but when lhs is VR_VARYING in -ffast-math, > i.e. minimum to maximum representable finite and both signs of NaN, > then set does all we need, we don't need to or in a NaN range). > We don't really later use the range in a way that would become a problem > that it is wider than varying, we actually just perform maths on the > two boundaries. > > As I said in the PR, this doesn't fix the !MODE_HAS_INFINITIES case, > I believe we actually need to treat the boundary values as infinities > in that case because they (probably) work like that, but it is unclear > if it is just the reverse operation lhs widening that is a problem there, > or whether it is a general problem. I have zero experience with > floating points without infinities (PDP11, some ARM half type?, > what else?). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2023-03-10 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/109008 > * range-op-float.cc (float_widen_lhs_range): If lb is > minimum representable finite number or ub is maximum > representable finite number, instead of widening it to > -inf or inf widen it to negative or positive 0x0.8p+(EMAX+1). > Temporarily clear flag_finite_math_only when canonicalizing > the widened range. > > * gcc.dg/pr109008.c: New test. > > --- gcc/range-op-float.cc.jj 2023-03-09 09:54:53.880453046 +0100 > +++ gcc/range-op-float.cc 2023-03-09 20:52:07.456284507 +0100 > @@ -2217,12 +2217,42 @@ float_widen_lhs_range (tree type, const > REAL_VALUE_TYPE lb = lhs.lower_bound (); > REAL_VALUE_TYPE ub = lhs.upper_bound (); > if (real_isfinite (&lb)) > - frange_nextafter (TYPE_MODE (type), lb, dconstninf); > + { > + frange_nextafter (TYPE_MODE (type), lb, dconstninf); > + if (real_isinf (&lb)) > + { > + /* For -DBL_MAX, instead of -Inf use > + nexttoward (-DBL_MAX, -LDBL_MAX) in a hypothetical > + wider type with the same mantissa precision but larger > + exponent range; it is outside of range of double values, > + but makes it clear it is just one ulp larger rather than > + infinite amount larger. */ > + lb = dconstm1; > + SET_REAL_EXP (&lb, FLOAT_MODE_FORMAT (TYPE_MODE (type))->emax + 1); > + } > + } > if (real_isfinite (&ub)) > - frange_nextafter (TYPE_MODE (type), ub, dconstinf); > + { > + frange_nextafter (TYPE_MODE (type), ub, dconstinf); > + if (real_isinf (&ub)) > + { > + /* For DBL_MAX similarly. */ > + ub = dconst1; > + SET_REAL_EXP (&ub, FLOAT_MODE_FORMAT (TYPE_MODE (type))->emax + 1); > + } > + } > + /* Temporarily disable -ffinite-math-only, so that frange::set doesn't > + reduce the range back to real_min_representable (type) as lower bound > + or real_max_representable (type) as upper bound. */ > + bool save_flag_finite_math_only = flag_finite_math_only; > + flag_finite_math_only = false; > ret.set (type, lb, ub); > - ret.clear_nan (); > - ret.union_ (lhs); > + if (lhs.kind () != VR_VARYING) > + { > + ret.clear_nan (); > + ret.union_ (lhs); > + } > + flag_finite_math_only = save_flag_finite_math_only; Meh - I wonder if we can avoid all this by making float_widen_lhs_range friend of frange and simply access m_min/m_max directly and use the copy-CTOR to copy bounds and nan state ... after all verify_range will likely fail after you restore flag_finite_math_only ... But OK for the moment. Thanks, Richard. > return ret; > } > > --- gcc/testsuite/gcc.dg/pr109008.c.jj 2023-03-09 12:25:11.507955698 +0100 > +++ gcc/testsuite/gcc.dg/pr109008.c 2023-03-09 12:33:35.795598344 +0100 > @@ -0,0 +1,26 @@ > +/* PR tree-optimization/109008 */ > +/* { dg-do run } */ > +/* { dg-options "-O2 -ffinite-math-only -fexcess-precision=standard" } */ > + > +__attribute__((noipa)) double > +foo (double eps) > +{ > + double d = __DBL_MAX__ + eps; > + if (d == __DBL_MAX__) > + if (eps > 16.0) > + return eps; > + return 0.0; > +} > + > +int > +main () > +{ > +#if __DBL_MANT_DIG__ == 53 && __DBL_MAX_EXP__ == 1024 && __DBL_MIN_EXP__ == -1021 \ > + && __FLT_EVAL_METHOD__ == 0 > + if (foo (0x0.8p+970) == 0.0) > + __builtin_abort (); > + if (foo (32.0) == 0.0) > + __builtin_abort (); > +#endif > + return 0; > +} > > Jakub > >
On Fri, Mar 10, 2023 at 08:53:37AM +0000, Richard Biener wrote: > Meh - I wonder if we can avoid all this by making float_widen_lhs_range > friend of frange and simply access m_min/m_max directly and use the > copy-CTOR to copy bounds and nan state ... after all verify_range > will likely fail after you restore flag_finite_math_only ... I'll defer such changes to Aldy. As for verification, I think verify_range will not fail on it, it mainly checks whether it is normalized (e.g. if minimum is frange_val_min and maximum is frange_val_max and NaNs are possible with both signs (if NaNs are supported) then it is VR_VARYING etc.). It doesn't check if the actual VR_RANGE bounds are smaller or larger than the VR_VARYING bounds, there is just equality check. Of course, behavior of wider than varying ranges is still unexpected in many ways, say the union_ of such a range and VR_VARYING will ICE etc. Now, I guess another possibility for the reverse ops over these wider ranges would be avoid calling fold_range in the reverse ops, but call rv_fold directly or have fold_range variant which would instead of the op1, op2 argument have 2 triplets, op1, op1lb, op1ub, op2, op2lb, op2ub, and it would use those const REAL_VALUE_TYPE &op??b in preference to op?.{lower,upper}_bound () or perhaps normal fold_range be implemented in terms of this extended fold_range. Then we wouldn't need to bother with these non-standard franges... > But OK for the moment. Thanks, committed. Jakub
On 3/10/23 11:29, Jakub Jelinek wrote: > On Fri, Mar 10, 2023 at 08:53:37AM +0000, Richard Biener wrote: >> Meh - I wonder if we can avoid all this by making float_widen_lhs_range >> friend of frange and simply access m_min/m_max directly and use the >> copy-CTOR to copy bounds and nan state ... after all verify_range >> will likely fail after you restore flag_finite_math_only ... > > I'll defer such changes to Aldy. > > As for verification, I think verify_range will not fail on it, it mainly > checks whether it is normalized (e.g. if minimum is frange_val_min and > maximum is frange_val_max and NaNs are possible with both signs (if NaNs > are supported) then it is VR_VARYING etc.). It doesn't check if the actual > VR_RANGE bounds are smaller or larger than the VR_VARYING bounds, there is > just equality check. > Of course, behavior of wider than varying ranges is still unexpected in > many ways, say the union_ of such a range and VR_VARYING will ICE etc. > > Now, I guess another possibility for the reverse ops over these wider ranges > would be avoid calling fold_range in the reverse ops, but call rv_fold > directly or have fold_range variant which would instead of the op1, op2 > argument have 2 triplets, op1, op1lb, op1ub, op2, op2lb, op2ub, and it > would use those const REAL_VALUE_TYPE &op??b in preference to > op?.{lower,upper}_bound () or perhaps normal fold_range be implemented > in terms of this extended fold_range. Then we wouldn't need to bother with > these non-standard franges... > >> But OK for the moment. > > Thanks, committed. I'm not a big fan of constructing ranges that break all our internal consistency checks. I'd also prefer to add another constructor (or add a flag to the current constructor) instead of making range-op-float routines friends. We also have bits in the vrange (or frange) that we could use for other semantics, especially since frange_storage can be streamlined when stored in GC/etc. I'm on PTO this week. Could we revisit this next week? And if worse comes to worse, leave it as is, and fix it properly next release? Thanks for your work on this. Aldy
On Mon, 13 Mar 2023, Aldy Hernandez wrote: > > > On 3/10/23 11:29, Jakub Jelinek wrote: > > On Fri, Mar 10, 2023 at 08:53:37AM +0000, Richard Biener wrote: > >> Meh - I wonder if we can avoid all this by making float_widen_lhs_range > >> friend of frange and simply access m_min/m_max directly and use the > >> copy-CTOR to copy bounds and nan state ... after all verify_range > >> will likely fail after you restore flag_finite_math_only ... > > > > I'll defer such changes to Aldy. > > > > As for verification, I think verify_range will not fail on it, it mainly > > checks whether it is normalized (e.g. if minimum is frange_val_min and > > maximum is frange_val_max and NaNs are possible with both signs (if NaNs > > are supported) then it is VR_VARYING etc.). It doesn't check if the actual > > VR_RANGE bounds are smaller or larger than the VR_VARYING bounds, there is > > just equality check. > > Of course, behavior of wider than varying ranges is still unexpected in > > many ways, say the union_ of such a range and VR_VARYING will ICE etc. > > > > Now, I guess another possibility for the reverse ops over these wider ranges > > would be avoid calling fold_range in the reverse ops, but call rv_fold > > directly or have fold_range variant which would instead of the op1, op2 > > argument have 2 triplets, op1, op1lb, op1ub, op2, op2lb, op2ub, and it > > would use those const REAL_VALUE_TYPE &op??b in preference to > > op?.{lower,upper}_bound () or perhaps normal fold_range be implemented > > in terms of this extended fold_range. Then we wouldn't need to bother with > > these non-standard franges... > > > >> But OK for the moment. > > > > Thanks, committed. > > I'm not a big fan of constructing ranges that break all our internal > consistency checks. I'd also prefer to add another constructor (or add a flag > to the current constructor) instead of making range-op-float routines friends. > We also have bits in the vrange (or frange) that we could use for other > semantics, especially since frange_storage can be streamlined when stored in > GC/etc. > > I'm on PTO this week. Could we revisit this next week? And if worse comes to > worse, leave it as is, and fix it properly next release? Yes, sure - I just noticed that we're forced to use high-level API for something that's quite low-level and should be internal (a range "breaking" internal consistency checks). Richard.
On 3/13/23 08:50, Richard Biener wrote: > On Mon, 13 Mar 2023, Aldy Hernandez wrote: > >> >> >> On 3/10/23 11:29, Jakub Jelinek wrote: >>> On Fri, Mar 10, 2023 at 08:53:37AM +0000, Richard Biener wrote: >>>> Meh - I wonder if we can avoid all this by making float_widen_lhs_range >>>> friend of frange and simply access m_min/m_max directly and use the >>>> copy-CTOR to copy bounds and nan state ... after all verify_range >>>> will likely fail after you restore flag_finite_math_only ... >>> >>> I'll defer such changes to Aldy. >>> >>> As for verification, I think verify_range will not fail on it, it mainly >>> checks whether it is normalized (e.g. if minimum is frange_val_min and >>> maximum is frange_val_max and NaNs are possible with both signs (if NaNs >>> are supported) then it is VR_VARYING etc.). It doesn't check if the actual >>> VR_RANGE bounds are smaller or larger than the VR_VARYING bounds, there is >>> just equality check. >>> Of course, behavior of wider than varying ranges is still unexpected in >>> many ways, say the union_ of such a range and VR_VARYING will ICE etc. >>> >>> Now, I guess another possibility for the reverse ops over these wider ranges >>> would be avoid calling fold_range in the reverse ops, but call rv_fold >>> directly or have fold_range variant which would instead of the op1, op2 >>> argument have 2 triplets, op1, op1lb, op1ub, op2, op2lb, op2ub, and it >>> would use those const REAL_VALUE_TYPE &op??b in preference to >>> op?.{lower,upper}_bound () or perhaps normal fold_range be implemented >>> in terms of this extended fold_range. Then we wouldn't need to bother with >>> these non-standard franges... >>> >>>> But OK for the moment. >>> >>> Thanks, committed. >> >> I'm not a big fan of constructing ranges that break all our internal >> consistency checks. I'd also prefer to add another constructor (or add a flag >> to the current constructor) instead of making range-op-float routines friends. >> We also have bits in the vrange (or frange) that we could use for other >> semantics, especially since frange_storage can be streamlined when stored in >> GC/etc. >> >> I'm on PTO this week. Could we revisit this next week? And if worse comes to >> worse, leave it as is, and fix it properly next release? > > Yes, sure - I just noticed that we're forced to use high-level API for > something that's quite low-level and should be internal (a range > "breaking" internal consistency checks). Yeah, let's fix the API. No sense hacking around things if what we need is to tweak the design. I don't like hacking around things. It always comes back to bite me ;-). Aldy
On Mon, Mar 13, 2023 at 08:59:15AM +0100, Aldy Hernandez wrote: > > Yes, sure - I just noticed that we're forced to use high-level API for > > something that's quite low-level and should be internal (a range > > "breaking" internal consistency checks). > > Yeah, let's fix the API. No sense hacking around things if what we need is > to tweak the design. > > I don't like hacking around things. It always comes back to bite me ;-). Sure. The current state is that I think the actual bugs are fixed except for the !MODE_HAS_INFINITIES case which people rarely use, so fixing up the API can wait even to next release. For !MODE_HAS_INFINITIES, I wonder if the best fix wouldn't be to change set and a few other spots, so that if the boundaries are real_min_representable/real_max_representable, we widen them to -inf and inf and change frange_val_min/max to also be dconstninf/dconstinf for !MODE_HAS_INFINITIES, because the min/max for that case (probably) really work as infinities. Whenever we actually round that value to mode, it will become real_min_representable/real_max_representable again. But that can also wait a week. Jakub
On 3/13/23 09:06, Jakub Jelinek wrote: > On Mon, Mar 13, 2023 at 08:59:15AM +0100, Aldy Hernandez wrote: >>> Yes, sure - I just noticed that we're forced to use high-level API for >>> something that's quite low-level and should be internal (a range >>> "breaking" internal consistency checks). >> >> Yeah, let's fix the API. No sense hacking around things if what we need is >> to tweak the design. >> >> I don't like hacking around things. It always comes back to bite me ;-). > > Sure. The current state is that I think the actual bugs are fixed except > for the !MODE_HAS_INFINITIES case which people rarely use, so fixing up the > API can wait even to next release. > > For !MODE_HAS_INFINITIES, I wonder if the best fix wouldn't be to change > set and a few other spots, so that if the boundaries are > real_min_representable/real_max_representable, we widen them to -inf and inf > and change frange_val_min/max to also be dconstninf/dconstinf for > !MODE_HAS_INFINITIES, because the min/max for that case (probably) really work as > infinities. Whenever we actually round that value to mode, it will become > real_min_representable/real_max_representable again. > But that can also wait a week. That sounds very reasonable. It would remove special casing and would make the code easier to read. For that matter, that was what I had in the original implementation. Aldy
On Mon, Mar 13, 2023 at 09:41:47AM +0100, Aldy Hernandez wrote: > On 3/13/23 09:06, Jakub Jelinek wrote: > > On Mon, Mar 13, 2023 at 08:59:15AM +0100, Aldy Hernandez wrote: > > > > Yes, sure - I just noticed that we're forced to use high-level API for > > > > something that's quite low-level and should be internal (a range > > > > "breaking" internal consistency checks). > > > > > > Yeah, let's fix the API. No sense hacking around things if what we need is > > > to tweak the design. > > > > > > I don't like hacking around things. It always comes back to bite me ;-). > > > > Sure. The current state is that I think the actual bugs are fixed except > > for the !MODE_HAS_INFINITIES case which people rarely use, so fixing up the > > API can wait even to next release. > > > > For !MODE_HAS_INFINITIES, I wonder if the best fix wouldn't be to change > > set and a few other spots, so that if the boundaries are > > real_min_representable/real_max_representable, we widen them to -inf and inf > > and change frange_val_min/max to also be dconstninf/dconstinf for > > !MODE_HAS_INFINITIES, because the min/max for that case (probably) really work as > > infinities. Whenever we actually round that value to mode, it will become > > real_min_representable/real_max_representable again. > > But that can also wait a week. > > That sounds very reasonable. It would remove special casing and would make > the code easier to read. For that matter, that was what I had in the > original implementation. I think we don't want to remove the special casing for -ffinite-math-only on types which do support infinities. Thinking further on it, perhaps for !MODE_HAS_INFINITIES a better fix would be to do something like the patch below. Consider say having a range of VAX float type: #define M0 -FLT_MAX #define M1 nextafterf (F0, FLT_MAX) #define M2 nextafterf (M1, FLT_MAX) [M2, M2] - [M0, M1] Or perhaps if one or both of the operands are in such a case a min and max, perform real_arithmetic recurse on the argument replaced with dconstninf/dconstinf and then depending on inf pick the mininum or maximum of the two results (and carefully think about what to do if both operands are min/max). 2023-03-20 Jakub Jelinek <jakub@redhat.com> * range-op-float.cc (frange_arithmetic): For !MODE_HAS_INFINITIES types, pretend operands with minimum or maximum values are actually infinities. --- gcc/range-op-float.cc.jj 2023-03-10 12:40:19.673108938 +0100 +++ gcc/range-op-float.cc 2023-03-20 16:58:36.604981486 +0100 @@ -313,8 +313,26 @@ frange_arithmetic (enum tree_code code, REAL_VALUE_TYPE value; enum machine_mode mode = TYPE_MODE (type); bool mode_composite = MODE_COMPOSITE_P (mode); + const REAL_VALUE_TYPE *pop1 = &op1; + const REAL_VALUE_TYPE *pop2 = &op2; - bool inexact = real_arithmetic (&value, code, &op1, &op2); + if (!MODE_HAS_INFINITIES (mode)) + { + // If mode doesn't have infinities, the minimum and maximum + // values are saturating. Pretend for real_arithmetic such + // values are actual infinities. real_convert will then + // canonicalize the result not to be an infinity. + if (frange_val_is_min (op1, type)) + pop1 = &dconstninf; + else if (frange_val_is_max (op1, type)) + pop1 = &dconstinf; + if (frange_val_is_min (op2, type)) + pop2 = &dconstninf; + else if (frange_val_is_max (op2, type)) + pop2 = &dconstinf; + } + + bool inexact = real_arithmetic (&value, code, pop1, pop2); real_convert (&result, mode, &value); // Be extra careful if there may be discrepancies between the Jakub
On Mon, Mar 20, 2023 at 5:14 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Mar 13, 2023 at 09:41:47AM +0100, Aldy Hernandez wrote: > > On 3/13/23 09:06, Jakub Jelinek wrote: > > > On Mon, Mar 13, 2023 at 08:59:15AM +0100, Aldy Hernandez wrote: > > > > > Yes, sure - I just noticed that we're forced to use high-level API for > > > > > something that's quite low-level and should be internal (a range > > > > > "breaking" internal consistency checks). > > > > > > > > Yeah, let's fix the API. No sense hacking around things if what we need is > > > > to tweak the design. > > > > > > > > I don't like hacking around things. It always comes back to bite me ;-). > > > > > > Sure. The current state is that I think the actual bugs are fixed except > > > for the !MODE_HAS_INFINITIES case which people rarely use, so fixing up the > > > API can wait even to next release. > > > > > > For !MODE_HAS_INFINITIES, I wonder if the best fix wouldn't be to change > > > set and a few other spots, so that if the boundaries are > > > real_min_representable/real_max_representable, we widen them to -inf and inf > > > and change frange_val_min/max to also be dconstninf/dconstinf for > > > !MODE_HAS_INFINITIES, because the min/max for that case (probably) really work as > > > infinities. Whenever we actually round that value to mode, it will become > > > real_min_representable/real_max_representable again. > > > But that can also wait a week. > > > > That sounds very reasonable. It would remove special casing and would make > > the code easier to read. For that matter, that was what I had in the > > original implementation. > > I think we don't want to remove the special casing for -ffinite-math-only > on types which do support infinities. > Thinking further on it, perhaps for !MODE_HAS_INFINITIES a better fix would > be to do something like the patch below. > Consider say having a range of VAX float type: > #define M0 -FLT_MAX > #define M1 nextafterf (F0, FLT_MAX) > #define M2 nextafterf (M1, FLT_MAX) > [M2, M2] - [M0, M1] > Or perhaps if one or both of the operands are in such a case a min and max, > perform real_arithmetic recurse on the argument replaced with > dconstninf/dconstinf and then depending on inf pick the mininum or maximum > of the two results (and carefully think about what to do if both operands > are min/max). LGTM. Aldy > > 2023-03-20 Jakub Jelinek <jakub@redhat.com> > > * range-op-float.cc (frange_arithmetic): For !MODE_HAS_INFINITIES > types, pretend operands with minimum or maximum values are actually > infinities. > > --- gcc/range-op-float.cc.jj 2023-03-10 12:40:19.673108938 +0100 > +++ gcc/range-op-float.cc 2023-03-20 16:58:36.604981486 +0100 > @@ -313,8 +313,26 @@ frange_arithmetic (enum tree_code code, > REAL_VALUE_TYPE value; > enum machine_mode mode = TYPE_MODE (type); > bool mode_composite = MODE_COMPOSITE_P (mode); > + const REAL_VALUE_TYPE *pop1 = &op1; > + const REAL_VALUE_TYPE *pop2 = &op2; > > - bool inexact = real_arithmetic (&value, code, &op1, &op2); > + if (!MODE_HAS_INFINITIES (mode)) > + { > + // If mode doesn't have infinities, the minimum and maximum > + // values are saturating. Pretend for real_arithmetic such > + // values are actual infinities. real_convert will then > + // canonicalize the result not to be an infinity. > + if (frange_val_is_min (op1, type)) > + pop1 = &dconstninf; > + else if (frange_val_is_max (op1, type)) > + pop1 = &dconstinf; > + if (frange_val_is_min (op2, type)) > + pop2 = &dconstninf; > + else if (frange_val_is_max (op2, type)) > + pop2 = &dconstinf; > + } > + > + bool inexact = real_arithmetic (&value, code, pop1, pop2); > real_convert (&result, mode, &value); > > // Be extra careful if there may be discrepancies between the > > > Jakub >
On 3/10/23 09:53, Richard Biener wrote: > On Fri, 10 Mar 2023, Jakub Jelinek wrote: Coming back to this... > /* Temporarily disable -ffinite-math-only, so that frange::set doesn't > reduce the range back to real_min_representable (type) as lower bound > or real_max_representable (type) as upper bound. */ > bool save_flag_finite_math_only = flag_finite_math_only; > flag_finite_math_only = false; > ret.set (type, lb, ub); > if (lhs.kind () != VR_VARYING) > { > ret.clear_nan (); > ret.union_ (lhs); > } > flag_finite_math_only = save_flag_finite_math_only; It looks like what you want to do is be able to create a range with a known NAN state, but without the setter reducing the range to min/max_representable. How about we enhance the API to provide: 1. Constructor with a known NAN state. 2. Setter with a flag to keep it from canonicalizing into min/max_representable. The flag in 2 could in the future be saved in the frange object to keep union and friends from further canonicalization. So the above could be written as: // Construct [lb, ub] with a known NAN state. frange tmp (lb, ub, lhs.get_nan_state ()); // Set RET without dropping/reducing the range to MIN/MAX. ret.set (tmp, FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION); An alternative is to allow the setter to set everything: ret.set (type, lb, ub, lhs.get_nan_state (), FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION); Would this work? I'd be happy to whip up something this week, or if preferred, leave it to the next release. Aldy
On Tue, Mar 21, 2023 at 02:28:31PM +0100, Aldy Hernandez wrote: > > /* Temporarily disable -ffinite-math-only, so that frange::set doesn't > > reduce the range back to real_min_representable (type) as lower bound > > or real_max_representable (type) as upper bound. */ > > bool save_flag_finite_math_only = flag_finite_math_only; > > flag_finite_math_only = false; > > ret.set (type, lb, ub); > > if (lhs.kind () != VR_VARYING) > > { > > ret.clear_nan (); > > ret.union_ (lhs); > > } > > flag_finite_math_only = save_flag_finite_math_only; > > It looks like what you want to do is be able to create a range with a known > NAN state, but without the setter reducing the range to > min/max_representable. > > How about we enhance the API to provide: > > 1. Constructor with a known NAN state. > 2. Setter with a flag to keep it from canonicalizing into > min/max_representable. > > The flag in 2 could in the future be saved in the frange object to keep > union and friends from further canonicalization. > > So the above could be written as: > > // Construct [lb, ub] with a known NAN state. > frange tmp (lb, ub, lhs.get_nan_state ()); > > // Set RET without dropping/reducing the range to MIN/MAX. > ret.set (tmp, FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION); > > An alternative is to allow the setter to set everything: > > ret.set (type, lb, ub, > lhs.get_nan_state (), > FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION); > > Would this work? I'd be happy to whip up something this week, or if > preferred, leave it to the next release. The latter would be better, I really don't need a temporary range in that spot, the union_ is only to copy the NaN state. Though, I think right now set actually doesn't do reduction to representable at all, all it does is equality compare bounds against the applicable boundaries and if both are equal and NaN state is appropriate, change it into VR_VARYING. So maybe for now all we need is the 4 argument set. Jakub
On 3/21/23 14:39, Jakub Jelinek wrote: > On Tue, Mar 21, 2023 at 02:28:31PM +0100, Aldy Hernandez wrote: >>> /* Temporarily disable -ffinite-math-only, so that frange::set doesn't >>> reduce the range back to real_min_representable (type) as lower bound >>> or real_max_representable (type) as upper bound. */ >>> bool save_flag_finite_math_only = flag_finite_math_only; >>> flag_finite_math_only = false; >>> ret.set (type, lb, ub); >>> if (lhs.kind () != VR_VARYING) >>> { >>> ret.clear_nan (); >>> ret.union_ (lhs); >>> } >>> flag_finite_math_only = save_flag_finite_math_only; >> >> It looks like what you want to do is be able to create a range with a known >> NAN state, but without the setter reducing the range to >> min/max_representable. >> >> How about we enhance the API to provide: >> >> 1. Constructor with a known NAN state. >> 2. Setter with a flag to keep it from canonicalizing into >> min/max_representable. >> >> The flag in 2 could in the future be saved in the frange object to keep >> union and friends from further canonicalization. >> >> So the above could be written as: >> >> // Construct [lb, ub] with a known NAN state. >> frange tmp (lb, ub, lhs.get_nan_state ()); >> >> // Set RET without dropping/reducing the range to MIN/MAX. >> ret.set (tmp, FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION); >> >> An alternative is to allow the setter to set everything: >> >> ret.set (type, lb, ub, >> lhs.get_nan_state (), >> FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION); >> >> Would this work? I'd be happy to whip up something this week, or if >> preferred, leave it to the next release. > > The latter would be better, I really don't need a temporary range in that > spot, the union_ is only to copy the NaN state. > Though, I think right now set actually doesn't do reduction to representable > at all, all it does is equality compare bounds against the applicable > boundaries and if both are equal and NaN state is appropriate, change it > into VR_VARYING. > So maybe for now all we need is the 4 argument set. So, this? frange::set (tree type, const REAL_VALUE_TYPE &min, const REAL_VALUE_TYPE &max, const nan_state &, value_range_kind kind = VR_RANGE) If so, I'll start poking at it. Aldy
On Tue, Mar 21, 2023 at 02:49:49PM +0100, Aldy Hernandez wrote: > So, this? > > frange::set (tree type, > const REAL_VALUE_TYPE &min, > const REAL_VALUE_TYPE &max, > const nan_state &, > value_range_kind kind = VR_RANGE) > > If so, I'll start poking at it. Yes. Jakub
On 3/21/23 14:56, Jakub Jelinek wrote: > On Tue, Mar 21, 2023 at 02:49:49PM +0100, Aldy Hernandez wrote: >> So, this? >> >> frange::set (tree type, >> const REAL_VALUE_TYPE &min, >> const REAL_VALUE_TYPE &max, >> const nan_state &, >> value_range_kind kind = VR_RANGE) >> >> If so, I'll start poking at it. > > Yes. > > Jakub > Tested on x86-64 Linux. OK? Aldy
On Wed, Mar 22, 2023 at 07:32:44AM +0100, Aldy Hernandez wrote: > On 3/21/23 14:56, Jakub Jelinek wrote: > > On Tue, Mar 21, 2023 at 02:49:49PM +0100, Aldy Hernandez wrote: > > > So, this? > > > > > > frange::set (tree type, > > > const REAL_VALUE_TYPE &min, > > > const REAL_VALUE_TYPE &max, > > > const nan_state &, > > > value_range_kind kind = VR_RANGE) > > > > > > If so, I'll start poking at it. > > > > Yes. > > > > Jakub > > > > Tested on x86-64 Linux. > > OK? Yes, thanks. Jakub
--- gcc/range-op-float.cc.jj 2023-03-09 09:54:53.880453046 +0100 +++ gcc/range-op-float.cc 2023-03-09 20:52:07.456284507 +0100 @@ -2217,12 +2217,42 @@ float_widen_lhs_range (tree type, const REAL_VALUE_TYPE lb = lhs.lower_bound (); REAL_VALUE_TYPE ub = lhs.upper_bound (); if (real_isfinite (&lb)) - frange_nextafter (TYPE_MODE (type), lb, dconstninf); + { + frange_nextafter (TYPE_MODE (type), lb, dconstninf); + if (real_isinf (&lb)) + { + /* For -DBL_MAX, instead of -Inf use + nexttoward (-DBL_MAX, -LDBL_MAX) in a hypothetical + wider type with the same mantissa precision but larger + exponent range; it is outside of range of double values, + but makes it clear it is just one ulp larger rather than + infinite amount larger. */ + lb = dconstm1; + SET_REAL_EXP (&lb, FLOAT_MODE_FORMAT (TYPE_MODE (type))->emax + 1); + } + } if (real_isfinite (&ub)) - frange_nextafter (TYPE_MODE (type), ub, dconstinf); + { + frange_nextafter (TYPE_MODE (type), ub, dconstinf); + if (real_isinf (&ub)) + { + /* For DBL_MAX similarly. */ + ub = dconst1; + SET_REAL_EXP (&ub, FLOAT_MODE_FORMAT (TYPE_MODE (type))->emax + 1); + } + } + /* Temporarily disable -ffinite-math-only, so that frange::set doesn't + reduce the range back to real_min_representable (type) as lower bound + or real_max_representable (type) as upper bound. */ + bool save_flag_finite_math_only = flag_finite_math_only; + flag_finite_math_only = false; ret.set (type, lb, ub); - ret.clear_nan (); - ret.union_ (lhs); + if (lhs.kind () != VR_VARYING) + { + ret.clear_nan (); + ret.union_ (lhs); + } + flag_finite_math_only = save_flag_finite_math_only; return ret; } --- gcc/testsuite/gcc.dg/pr109008.c.jj 2023-03-09 12:25:11.507955698 +0100 +++ gcc/testsuite/gcc.dg/pr109008.c 2023-03-09 12:33:35.795598344 +0100 @@ -0,0 +1,26 @@ +/* PR tree-optimization/109008 */ +/* { dg-do run } */ +/* { dg-options "-O2 -ffinite-math-only -fexcess-precision=standard" } */ + +__attribute__((noipa)) double +foo (double eps) +{ + double d = __DBL_MAX__ + eps; + if (d == __DBL_MAX__) + if (eps > 16.0) + return eps; + return 0.0; +} + +int +main () +{ +#if __DBL_MANT_DIG__ == 53 && __DBL_MAX_EXP__ == 1024 && __DBL_MIN_EXP__ == -1021 \ + && __FLT_EVAL_METHOD__ == 0 + if (foo (0x0.8p+970) == 0.0) + __builtin_abort (); + if (foo (32.0) == 0.0) + __builtin_abort (); +#endif + return 0; +}