From: Aldy Hernandez <aldyh@redhat.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Andrew MacLeod <amacleod@redhat.com>,
gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] range-op-float: Fix reverse ops of comparisons [PR109386]
Date: Tue, 4 Apr 2023 16:08:17 +0200 [thread overview]
Message-ID: <CAGm3qMVu2jTxGjtEQtaSx-PbfUbjSSzq4a36Ac1FJ9ZUbBpgGQ@mail.gmail.com> (raw)
In-Reply-To: <ZCsu1qEUuowRqlWf@tucnak>
[-- Attachment #1: Type: text/plain, Size: 9230 bytes --]
OK.
On Mon, Apr 3, 2023, 21:54 Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> I've missed one of my recent range-op-float.cc changes (likely the
> r13-6967 one) caused
> FAIL: libphobos.phobos/std/math/algebraic.d execution test
> FAIL: libphobos.phobos_shared/std/math/algebraic.d execution test
> regressions, distilled into a C testcase below.
>
> In the testcase, we have
> !(u >= v)
> condition where both u and v are results of fabs*, which guards
> t1 = u u<= __FLT_MAX__;
> and
> t2 = v u<= __FLT_MAX__;
> comparisons. From false u >= v where u and v have [0.0, +Inf] NAN
> ranges we (incorrectly deduce that one of them is [nextafterf (0.0, 1.0),
> +Inf] NAN
> and the other is [0.0, nextafterf (+Inf, 0.0)] NAN and from that deduce
> that
> one of the comparisons is always true, because UNLE_EXPR with the maximum
> representable number are false only if the value is +Inf and our ranges
> tell
> that is not possible.
>
> The bug is that the u >= v comparison determines a sensible range only when
> it is true - we then know neither operand can be NAN and it behaves
> correctly. But when the comparison is false, our current code gives
> sensible answers only if the other op can't be NAN. If it can be NAN,
> whenever it is NAN, the comparison is always false regardless of the other
> value, so the other value needs to be VARYING.
> Now, foperator_unordered_lt::op1_range etc. had code to deal with that
> for op?.known_nan (), but as the testcase shows, it is enough if it may be
> a
> NAN at runtime to make it VARYING.
>
> So, the following patch replaces for all those BRS_FALSE cases of the
> normal
> non-equality comparisons if (opOTHER.known_isnan ()) r.set_varying (type);
> to do it also if maybe_isnan ().
>
> For the unordered or ... comparisons, it is similar for BRS_TRUE. Those
> comparisons are true whenever either operand is NAN, or if neither is NAN,
> the corresponding normal comparison. So, if those comparisons are true
> and other operand might be a NAN, we can't tell (VARYING), if it is false,
> currently handling is correct.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, fixes those 2
> D testcases and the newly added one. Ok for trunk?
>
> 2023-04-03 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/109386
> * range-op-float.cc (foperator_lt::op1_range,
> foperator_lt::op2_range,
> foperator_le::op1_range, foperator_le::op2_range,
> foperator_gt::op1_range, foperator_gt::op2_range,
> foperator_ge::op1_range, foperator_ge::op2_range): Make r varying
> for
> BRS_FALSE case even if the other op is maybe_isnan, not just
> known_isnan.
> (foperator_unordered_lt::op1_range,
> foperator_unordered_lt::op2_range,
> foperator_unordered_le::op1_range,
> foperator_unordered_le::op2_range,
> foperator_unordered_gt::op1_range,
> foperator_unordered_gt::op2_range,
> foperator_unordered_ge::op1_range,
> foperator_unordered_ge::op2_range):
> Make r varying for BRS_TRUE case even if the other op is
> maybe_isnan,
> not just known_isnan.
>
> * gcc.c-torture/execute/ieee/pr109386.c: New test.
>
> --- gcc/range-op-float.cc.jj 2023-04-03 10:42:54.000000000 +0200
> +++ gcc/range-op-float.cc 2023-04-03 13:31:01.163216123 +0200
> @@ -889,7 +889,7 @@ foperator_lt::op1_range (frange &r,
>
> case BRS_FALSE:
> // On the FALSE side of x < NAN, we know nothing about x.
> - if (op2.known_isnan ())
> + if (op2.known_isnan () || op2.maybe_isnan ())
> r.set_varying (type);
> else
> build_ge (r, type, op2);
> @@ -926,7 +926,7 @@ foperator_lt::op2_range (frange &r,
>
> case BRS_FALSE:
> // On the FALSE side of NAN < x, we know nothing about x.
> - if (op1.known_isnan ())
> + if (op1.known_isnan () || op1.maybe_isnan ())
> r.set_varying (type);
> else
> build_le (r, type, op1);
> @@ -1005,7 +1005,7 @@ foperator_le::op1_range (frange &r,
>
> case BRS_FALSE:
> // On the FALSE side of x <= NAN, we know nothing about x.
> - if (op2.known_isnan ())
> + if (op2.known_isnan () || op2.maybe_isnan ())
> r.set_varying (type);
> else
> build_gt (r, type, op2);
> @@ -1038,7 +1038,7 @@ foperator_le::op2_range (frange &r,
>
> case BRS_FALSE:
> // On the FALSE side of NAN <= x, we know nothing about x.
> - if (op1.known_isnan ())
> + if (op1.known_isnan () || op1.maybe_isnan ())
> r.set_varying (type);
> else if (op1.undefined_p ())
> return false;
> @@ -1122,7 +1122,7 @@ foperator_gt::op1_range (frange &r,
>
> case BRS_FALSE:
> // On the FALSE side of x > NAN, we know nothing about x.
> - if (op2.known_isnan ())
> + if (op2.known_isnan () || op2.maybe_isnan ())
> r.set_varying (type);
> else if (op2.undefined_p ())
> return false;
> @@ -1161,7 +1161,7 @@ foperator_gt::op2_range (frange &r,
>
> case BRS_FALSE:
> // On The FALSE side of NAN > x, we know nothing about x.
> - if (op1.known_isnan ())
> + if (op1.known_isnan () || op1.maybe_isnan ())
> r.set_varying (type);
> else if (op1.undefined_p ())
> return false;
> @@ -1241,7 +1241,7 @@ foperator_ge::op1_range (frange &r,
>
> case BRS_FALSE:
> // On the FALSE side of x >= NAN, we know nothing about x.
> - if (op2.known_isnan ())
> + if (op2.known_isnan () || op2.maybe_isnan ())
> r.set_varying (type);
> else if (op2.undefined_p ())
> return false;
> @@ -1275,7 +1275,7 @@ foperator_ge::op2_range (frange &r, tree
>
> case BRS_FALSE:
> // On the FALSE side of NAN >= x, we know nothing about x.
> - if (op1.known_isnan ())
> + if (op1.known_isnan () || op1.maybe_isnan ())
> r.set_varying (type);
> else if (op1.undefined_p ())
> return false;
> @@ -1639,7 +1639,7 @@ foperator_unordered_lt::op1_range (frang
> switch (get_bool_state (r, lhs, type))
> {
> case BRS_TRUE:
> - if (op2.known_isnan ())
> + if (op2.known_isnan () || op2.maybe_isnan ())
> r.set_varying (type);
> else if (op2.undefined_p ())
> return false;
> @@ -1673,7 +1673,7 @@ foperator_unordered_lt::op2_range (frang
> switch (get_bool_state (r, lhs, type))
> {
> case BRS_TRUE:
> - if (op1.known_isnan ())
> + if (op1.known_isnan () || op1.maybe_isnan ())
> r.set_varying (type);
> else if (op1.undefined_p ())
> return false;
> @@ -1755,7 +1755,7 @@ foperator_unordered_le::op1_range (frang
> switch (get_bool_state (r, lhs, type))
> {
> case BRS_TRUE:
> - if (op2.known_isnan ())
> + if (op2.known_isnan () || op2.maybe_isnan ())
> r.set_varying (type);
> else if (op2.undefined_p ())
> return false;
> @@ -1788,7 +1788,7 @@ foperator_unordered_le::op2_range (frang
> switch (get_bool_state (r, lhs, type))
> {
> case BRS_TRUE:
> - if (op1.known_isnan ())
> + if (op1.known_isnan () || op1.maybe_isnan ())
> r.set_varying (type);
> else if (op1.undefined_p ())
> return false;
> @@ -1872,7 +1872,7 @@ foperator_unordered_gt::op1_range (frang
> switch (get_bool_state (r, lhs, type))
> {
> case BRS_TRUE:
> - if (op2.known_isnan ())
> + if (op2.known_isnan () || op2.maybe_isnan ())
> r.set_varying (type);
> else if (op2.undefined_p ())
> return false;
> @@ -1907,7 +1907,7 @@ foperator_unordered_gt::op2_range (frang
> switch (get_bool_state (r, lhs, type))
> {
> case BRS_TRUE:
> - if (op1.known_isnan ())
> + if (op1.known_isnan () || op1.maybe_isnan ())
> r.set_varying (type);
> else if (op1.undefined_p ())
> return false;
> @@ -1991,7 +1991,7 @@ foperator_unordered_ge::op1_range (frang
> switch (get_bool_state (r, lhs, type))
> {
> case BRS_TRUE:
> - if (op2.known_isnan ())
> + if (op2.known_isnan () || op2.maybe_isnan ())
> r.set_varying (type);
> else if (op2.undefined_p ())
> return false;
> @@ -2025,7 +2025,7 @@ foperator_unordered_ge::op2_range (frang
> switch (get_bool_state (r, lhs, type))
> {
> case BRS_TRUE:
> - if (op1.known_isnan ())
> + if (op1.known_isnan () || op1.maybe_isnan ())
> r.set_varying (type);
> else if (op1.undefined_p ())
> return false;
> --- gcc/testsuite/gcc.c-torture/execute/ieee/pr109386.c.jj 2023-04-03
> 13:44:59.715981917 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/ieee/pr109386.c 2023-04-03
> 13:44:48.480145842 +0200
> @@ -0,0 +1,21 @@
> +/* PR tree-optimization/109386 */
> +
> +static inline float
> +foo (float x, float y)
> +{
> + float u = __builtin_fabsf (x);
> + float v = __builtin_fabsf (y);
> + if (!(u >= v))
> + {
> + if (__builtin_isinf (v)) return v;
> + if (__builtin_isinf (u)) return u;
> + }
> + return 42.0f;
> +}
> +
> +int
> +main ()
> +{
> + if (!__builtin_isinf (foo (__builtin_inff (), __builtin_nanf (""))))
> + __builtin_abort ();
> +}
>
> Jakub
>
>
prev parent reply other threads:[~2023-04-04 14:08 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-03 19:53 Jakub Jelinek
2023-04-04 14:08 ` Aldy Hernandez [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGm3qMVu2jTxGjtEQtaSx-PbfUbjSSzq4a36Ac1FJ9ZUbBpgGQ@mail.gmail.com \
--to=aldyh@redhat.com \
--cc=amacleod@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).