diff mbox series

PR tree-optimization/108447 - Do not try to logical fold floating point relations.

Message ID 0780922f-99b5-96fc-b921-9e00652f9741@redhat.com
State Committed
Commit ec5e99e95954fd629283a9c9572193dd95471fea
Headers
Series PR tree-optimization/108447 - Do not try to logical fold floating point relations. |

Commit Message

Andrew MacLeod Jan. 25, 2023, 6:05 p.m. UTC 
  This boils down to a single place where we are trying to do things with 
relations in ranger that are simply not safe when we need to honor NANs.

I think we can avoid all the other shuffling of relations, and simply 
not perform this optimization when it comes to floats.

The case the routine handles is:

c_2 = a_6 > b_7
c_3 = a_6 < b_7
c_4 = c_2 && c_3

c_2 and c_3 can never be true at the same time, Therefore c_4 can always 
resolve to false based purely on the relations.


Range-ops is unable to do this optimization directly as it requires 
examining things from outside the statement, and is not easily 
communicated a simple relation to operator_logical_and.

This routine proceeds to look at the definitions of c_2 and c_3, tries 
to determine if there are common operands, and queries for any relations 
between them.   If it turns out there is something, depending on whether 
its && or || , we use intersection or union to determine if the result 
of the logical operation can be folded.  If HONOR_NANS is true for the 
float type, then we cannot do this optimization, and bail early.

At this point I do not think we need to do any of the other things 
proposed to relations, so we don't need either of the other 2 patches 
this release.

Bootstraps on x86_64-pc-linux-gnu with no regressions.  OK for trunk?

Andrew
commit 0b080e9579045dd054e9b3289d123d6b66567e3e
Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Wed Jan 25 11:13:23 2023 -0500

    Do not try to logical fold floating point relations.
    
    relation_fold_and_or looks for relations among common operands feeding
    logical ands and ors.  With no knowledge of NANs, it should not attempt
    to do this with floating point ssa names.
    
            PR tree-optimization/108447
            gcc/
            * gimple-range-fold.cc (old_using_range::relation_fold_and_or):
            Do not attempt to fold HONOR_NAN types.
    
            gcc/testsuite/
            * gcc.dg/pr108447.c: New.

Comments

Richard Biener Jan. 26, 2023, 7:09 a.m. UTC | #1 
On Wed, Jan 25, 2023 at 7:05 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>
> This boils down to a single place where we are trying to do things with
> relations in ranger that are simply not safe when we need to honor NANs.
>
> I think we can avoid all the other shuffling of relations, and simply
> not perform this optimization when it comes to floats.
>
> The case the routine handles is:
>
> c_2 = a_6 > b_7
> c_3 = a_6 < b_7
> c_4 = c_2 && c_3
>
> c_2 and c_3 can never be true at the same time, Therefore c_4 can always
> resolve to false based purely on the relations.
>
>
> Range-ops is unable to do this optimization directly as it requires
> examining things from outside the statement, and is not easily
> communicated a simple relation to operator_logical_and.
>
> This routine proceeds to look at the definitions of c_2 and c_3, tries
> to determine if there are common operands, and queries for any relations
> between them.   If it turns out there is something, depending on whether
> its && or || , we use intersection or union to determine if the result
> of the logical operation can be folded.  If HONOR_NANS is true for the
> float type, then we cannot do this optimization, and bail early.
>
> At this point I do not think we need to do any of the other things
> proposed to relations, so we don't need either of the other 2 patches
> this release.
>
> Bootstraps on x86_64-pc-linux-gnu with no regressions.  OK for trunk?

+  if (HONOR_NANS (TREE_TYPE (ssa1_dep1)))
+    return;

would that rather be !(range-includes-nan (ssa1_dep1) ||
range-includes-nan (ssa1_dep2) || ..)?

That said, if the other 2 patches fix some latent issues in the new
frange code I'd
rather have them fixed.

Richard.

>
> Andrew
Richard Biener Jan. 26, 2023, 7:13 a.m. UTC | #2 
On Thu, Jan 26, 2023 at 8:09 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Wed, Jan 25, 2023 at 7:05 PM Andrew MacLeod <amacleod@redhat.com> wrote:
> >
> > This boils down to a single place where we are trying to do things with
> > relations in ranger that are simply not safe when we need to honor NANs.
> >
> > I think we can avoid all the other shuffling of relations, and simply
> > not perform this optimization when it comes to floats.
> >
> > The case the routine handles is:
> >
> > c_2 = a_6 > b_7
> > c_3 = a_6 < b_7
> > c_4 = c_2 && c_3
> >
> > c_2 and c_3 can never be true at the same time, Therefore c_4 can always
> > resolve to false based purely on the relations.
> >
> >
> > Range-ops is unable to do this optimization directly as it requires
> > examining things from outside the statement, and is not easily
> > communicated a simple relation to operator_logical_and.
> >
> > This routine proceeds to look at the definitions of c_2 and c_3, tries
> > to determine if there are common operands, and queries for any relations
> > between them.   If it turns out there is something, depending on whether
> > its && or || , we use intersection or union to determine if the result
> > of the logical operation can be folded.  If HONOR_NANS is true for the
> > float type, then we cannot do this optimization, and bail early.
> >
> > At this point I do not think we need to do any of the other things
> > proposed to relations, so we don't need either of the other 2 patches
> > this release.
> >
> > Bootstraps on x86_64-pc-linux-gnu with no regressions.  OK for trunk?
>
> +  if (HONOR_NANS (TREE_TYPE (ssa1_dep1)))
> +    return;
>
> would that rather be !(range-includes-nan (ssa1_dep1) ||
> range-includes-nan (ssa1_dep2) || ..)?

Saw the discussion in the other thread only now, so OK.

> That said, if the other 2 patches fix some latent issues in the new
> frange code I'd
> rather have them fixed.

So do we know bugs in the current code?  You said some buggy
function isn't used, so better delete it.  Are there other latent issues?

Thanks,
Richard.

> Richard.
>
> >
> > Andrew
Andrew MacLeod Jan. 26, 2023, 2:24 p.m. UTC | #3 
On 1/26/23 02:13, Richard Biener wrote:
> On Thu, Jan 26, 2023 at 8:09 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jan 25, 2023 at 7:05 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>>> This boils down to a single place where we are trying to do things with
>>> relations in ranger that are simply not safe when we need to honor NANs.
>>>
>>> I think we can avoid all the other shuffling of relations, and simply
>>> not perform this optimization when it comes to floats.
>>>
>>> The case the routine handles is:
>>>
>>> c_2 = a_6 > b_7
>>> c_3 = a_6 < b_7
>>> c_4 = c_2 && c_3
>>>
>>> c_2 and c_3 can never be true at the same time, Therefore c_4 can always
>>> resolve to false based purely on the relations.
>>>
>>>
>>> Range-ops is unable to do this optimization directly as it requires
>>> examining things from outside the statement, and is not easily
>>> communicated a simple relation to operator_logical_and.
>>>
>>> This routine proceeds to look at the definitions of c_2 and c_3, tries
>>> to determine if there are common operands, and queries for any relations
>>> between them.   If it turns out there is something, depending on whether
>>> its && or || , we use intersection or union to determine if the result
>>> of the logical operation can be folded.  If HONOR_NANS is true for the
>>> float type, then we cannot do this optimization, and bail early.
>>>
>>> At this point I do not think we need to do any of the other things
>>> proposed to relations, so we don't need either of the other 2 patches
>>> this release.
>>>
>>> Bootstraps on x86_64-pc-linux-gnu with no regressions.  OK for trunk?
>> +  if (HONOR_NANS (TREE_TYPE (ssa1_dep1)))
>> +    return;
>>
>> would that rather be !(range-includes-nan (ssa1_dep1) ||
>> range-includes-nan (ssa1_dep2) || ..)?
> Saw the discussion in the other thread only now, so OK.
>
>> That said, if the other 2 patches fix some latent issues in the new
>> frange code I'd
>> rather have them fixed.
> So do we know bugs in the current code?  You said some buggy
> function isn't used, so better delete it.  Are there other latent issues?
>
No bugs :-) At leats not related tot hat.    Yes, negate turns out to 
not currently be used (im sure it will eventually), but without the 
VREL_OTHER or other changes to relation representation, the negate table 
is currently correct.

Andrew
diff mbox series

Patch

diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
index 91eb6298254..9c5359a3fc6 100644
--- a/gcc/gimple-range-fold.cc
+++ b/gcc/gimple-range-fold.cc
@@ -1039,6 +1039,9 @@  fold_using_range::relation_fold_and_or (irange& lhs_range, gimple *s,
   if (!ssa1_dep1 || !ssa1_dep2 || !ssa2_dep1 || !ssa2_dep2)
     return;
 
+  if (HONOR_NANS (TREE_TYPE (ssa1_dep1)))
+    return;
+
   // Make sure they are the same dependencies, and detect the order of the
   // relationship.
   bool reverse_op2 = true;
diff --git a/gcc/testsuite/gcc.dg/pr108447.c b/gcc/testsuite/gcc.dg/pr108447.c
new file mode 100644
index 00000000000..cfbaba6d0aa
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr108447.c
@@ -0,0 +1,33 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+__attribute__((noipa)) int
+foo (float x, float y)
+{
+  _Bool cmp1 = x <= y;
+  _Bool cmp2 = x >= y;
+  if (cmp1 && cmp2)
+    return 1;
+  else if (!cmp1 && !cmp2)
+    return -1;
+  return 0;
+}
+
+int
+main ()
+{
+  if (foo (0.0f, __builtin_nanf ("")) != -1)
+    __builtin_abort ();
+  if (foo (__builtin_nanf (""), -42.0f) != -1)
+    __builtin_abort ();
+  if (foo (0.0f, -0.0f) != 1)
+    __builtin_abort ();
+  if (foo (42.0f, 42.0f) != 1)
+    __builtin_abort ();
+  if (foo (42.0f, -0.0f) != 0)
+    __builtin_abort ();
+  if (foo (0.0f, -42.0f) != 0)
+    __builtin_abort ();
+  return 0;
+}
+