On 1/24/23 05:05, Jakub Jelinek wrote:
On Mon, Jan 23, 2023 at 12:44:48PM -0500, Andrew MacLeod wrote:
@@ -784,17 +794,28 @@ value_relation::negate ()
bool
value_relation::intersect (const value_relation &p)
{
- // Save previous value
- relation_kind old = related;
+ relation_kind k;
if (p.op1 () == op1 () && p.op2 () == op2 ())
- related = relation_intersect (kind (), p.kind ());
+ k = relation_intersect (kind (), p.kind ());
else if (p.op2 () == op1 () && p.op1 () == op2 ())
- related = relation_intersect (kind (), relation_swap (p.kind ()));
+ k = relation_intersect (kind (), relation_swap (p.kind ()));
else
return false;
- return old != related;
+ if (related == k)
+ return false;
+
+ bool float_p = name1 && FLOAT_TYPE_P (TREE_TYPE (name1));
+ if (float_p && p.kind () != k && k == VREL_UNDEFINED)
+ {
+ if (relation_lt_le_gt_ge_p (kind ())
+ || relation_lt_le_gt_ge_p (p.kind ()))
+ k = VREL_OTHER;
+ }
I don't understand this.
When at least one of the relations is VREL_{L,G}{T,E} and intersection
is VREL_UNDEFINED, it means other relation is VREL_UNDEFINED (but then
VREL_UNDEFINED is the right range to return), or one is VREL_{L,G}T
and the other is VREL_EQ (x > y && x == y, again, VREL_UNDEFINED is
the right answer, no ordered pair of x and y is greater (or less) and
equal at the same time and for unordered (at least one NaN) both
relations are false), or it is VREL_LT vs. VREL_G{T,E} or vice versa
or VREL_GT vs. VREL_L{T,E} or vice versa
(x < y && x >= y again, for ordered pairs of x and y it is never
true, if any is NaN, neither comparison is true).
I don't think you need to do anything floating point related in intersect.
It might seem to be inconsistent with union, but that is just because
VREL_OTHER is the union of VREL_LTGT, VREL_ORDERED, VREL_UNORDERED,
VREL_UNLT, VREL_UNGT, VREL_UNGT, VREL_UNGE and VREL_UNEQ, if we had
those 8 in addition to the current 8 non-PE ones you'd see that for
intersections one only gets the new codes when fp with possible
NANs and at least one of the intersection operand is one of the new
codes. In the VREL_OTHER world, simply VREL_OTHER intersected with
anything but VREL_UNDEFINED is VREL_OTHER.
That is the way VREL_OTHER is implemented in the table.
So the problem is not on the true side of the IF condition as in your
example, its on the false side. We see this commonly in code like this
if (x <= y) // FALSE side registers x > y
blah()
else if (x >= y) // FALSE side registers x < y
blah()
else
// Here we get VREL_UNDEFINED.
But for floating point, that is not true, this would be a known NAN
based on my understanding of this. If I understand correctly, the
condition on the true side is explicitly true, but the false side is
the "false condition Union NAN".. which we cant represent. but I
don't think we want to always make that VREL_OTHER either.
So it seems we have to do one of two things... either never produce
UNDEFINED for floating point (unless one operand is explicitly UNDEFINED
already), or on the false side of a condition, always produce a
VREL_OTHER since its a relation and a NAN possible. Of course, if the
floating point code for the range-op relation operator knows the false
side has no NAN, it could produce the appropriate relation then.
It seems to me the "easiest" to get right is to stop producing
VREL_UNDEFINED for intersection?
Or am I reading something wrong?
+ // (x < y) || (x > y) produces x != y, but this is not true with floats.
+ // (x <= y) || (x >= y) produces VARYING, which is also not true for floats.
This misses a couple of needed cases both in this comment and
in the actual implementation. In particular, the first comment
line is right, x < y || x > y is the only one where we get
VREL_NE and want VREL_OTHER (VREL_LTGT maybe later).
x <= y || x >= y is just one of the cases which produce VREL_VARYING
and we want VREL_OTHER (VREL_ORDERED maybe later) though,
x < y || x >= y
x <= y || x > y
are the others where from the current tables you also get
VREL_VARYING but want VREL_OTHER (VREL_ORDERED eventually).
+ // As they cannot be properly represented, use VREL_OTHER.
+ bool float_p = name1 && FLOAT_TYPE_P (TREE_TYPE (name1));
This should be
bool float_p = name1 && HONOR_NANS (TREE_TYPE (name1));
If it is not floating point mode, it will be false, if it is
floating point mode which doesn't have NANs in hw, it also behaves
the same relation-wise, and lastly if hw supports NANs but user
uses -ffast-math or -ffinite-math-only, the user guaranteed that
the operands will never be NAN (nor +-INF), which means that
again relation behave like the integral/pointer ones, there is
no 4th possible outcome of a comparison.
+ if (float_p && p.kind () != k)
+ {
+ if (kind () == VREL_LT && p.kind () == VREL_GT)
+ k = VREL_OTHER;
+ else if (kind () == VREL_GT && p.kind () == VREL_LT)
+ k = VREL_OTHER;
+ else if (kind () == VREL_LE && p.kind () == VREL_GE)
+ k = VREL_OTHER;
+ else if (kind () == VREL_GE && p.kind () == VREL_LE)
+ k = VREL_OTHER;
And as written above, this misses the VREL_LT vs. VREL_GE or
VREL_GE vs. VREL_LT or VREL_GT vs. VREL_LE or VREL_LE vs. VREL_GT
cases.
I think it is easier written as
if (k == VREL_VARYING
&& relation_lt_le_gt_ge_p (kind ())
&& relation_lt_le_gt_ge_p (p.kind ()))
k = VREL_OTHER;
Only when/if we'll want to differentiate between VREL_LTGT for
VREL_LT vs. VREL_GT or VREL_GT vs. VREL_LT and VREL_ORDERED
for the others we will need something different.
OK, thats the way I originally had it, but for some reason thought it
was only these cases. I shall fix. I still want to make sure that if
one of the operands is already VARYING, it also remains VARYING.
--- a/gcc/value-relation.h
+++ b/gcc/value-relation.h
@@ -70,6 +70,7 @@ typedef enum relation_kind_t
VREL_GE, // r1 >= r2
VREL_EQ, // r1 == r2
VREL_NE, // r1 != r2
+ VREL_OTHER, // unrepresentatble floating point relation.
s/unrepresentatble/unrepresentable/
VREL_PE8, // 8 bit partial equivalency
VREL_PE16, // 16 bit partial equivalency
VREL_PE32, // 32 bit partial equivalency
Otherwise LGTM (i.e. I agree with VREL_OTHER introduction for now)
and the intersect/union tables look good too (I must say I don't understand
much the transitive table - how that compares to the intersect one, but
that isn't problem in the code, just on my side).
The transitive table simply adds an ERROR_MARK for VREL_OTHER, which
means it will apply no transitivity if it sees VREL_OTHER.
Attached is an adjusted patch with your comments. Plus I left in the
intersection generally disallowing UNDEFINED as I'm discussed above. If
I have failed to convince you that is needed, then I am probably just
misunderstanding and I will pull it out :-)
Andrew
commit ad69e672be4b5adcbda074601d7bfbeac8206a6a
Author: Andrew MacLeod <amacl...@redhat.com>
Date: Fri Jan 20 17:31:26 2023 -0500
Add VREL_OTHER for FP unsupported relations.
Add VREL_OTHER to represent floating point relations we do not yet
support.
PR tree-optimization/108447
gcc/
* value-relation.cc (kind_string): Add "unknown fp".
(rr_negate_table): Add entry for VREL_OTHER.
(rr_swap_table): Likewise.
(rr_intersect_table): Likewise.
(rr_union_table): Likewise.
(rr_transitive_table): Likewise.
(relation_to_code): Likewise.
(value_relation::intersect): Handle floating point differences.
(value_relation::union_): Likewise.
* value-relation.h (enum relation_kind_t): Add VREL_OTHER.
gcc/testsuite/
* gcc.dg/pr108447.c: New.
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;
+}
+
diff --git a/gcc/value-relation.cc b/gcc/value-relation.cc
index 432828e2b13..2460fdd30f2 100644
--- a/gcc/value-relation.cc
+++ b/gcc/value-relation.cc
@@ -33,8 +33,8 @@ along with GCC; see the file COPYING3. If not see
#include "dominance.h"
static const char *kind_string[VREL_LAST] =
-{ "varying", "undefined", "<", "<=", ">", ">=", "==", "!=", "pe8", "pe16",
- "pe32", "pe64" };
+{ "varying", "undefined", "<", "<=", ">", ">=", "==", "!=", "unknown fp",
+ "pe8", "pe16", "pe32", "pe64" };
// Print a relation_kind REL to file F.
@@ -47,7 +47,7 @@ print_relation (FILE *f, relation_kind rel)
// This table is used to negate the operands. op1 REL op2 -> !(op1 REL op2).
relation_kind rr_negate_table[VREL_LAST] = {
VREL_VARYING, VREL_UNDEFINED, VREL_GE, VREL_GT, VREL_LE, VREL_LT, VREL_NE,
- VREL_EQ };
+ VREL_EQ, VREL_OTHER };
// Negate the relation, as in logical negation.
@@ -60,7 +60,7 @@ relation_negate (relation_kind r)
// This table is used to swap the operands. op1 REL op2 -> op2 REL op1.
relation_kind rr_swap_table[VREL_LAST] = {
VREL_VARYING, VREL_UNDEFINED, VREL_GT, VREL_GE, VREL_LT, VREL_LE, VREL_EQ,
- VREL_NE };
+ VREL_NE, VREL_OTHER };
// Return the relation as if the operands were swapped.
@@ -75,28 +75,32 @@ relation_swap (relation_kind r)
relation_kind rr_intersect_table[VREL_LAST][VREL_LAST] = {
// VREL_VARYING
{ VREL_VARYING, VREL_UNDEFINED, VREL_LT, VREL_LE, VREL_GT, VREL_GE, VREL_EQ,
- VREL_NE },
+ VREL_NE, VREL_OTHER },
// VREL_UNDEFINED
{ VREL_UNDEFINED, VREL_UNDEFINED, VREL_UNDEFINED, VREL_UNDEFINED,
- VREL_UNDEFINED, VREL_UNDEFINED, VREL_UNDEFINED, VREL_UNDEFINED },
+ VREL_UNDEFINED, VREL_UNDEFINED, VREL_UNDEFINED, VREL_UNDEFINED,
+ VREL_UNDEFINED },
// VREL_LT
{ VREL_LT, VREL_UNDEFINED, VREL_LT, VREL_LT, VREL_UNDEFINED, VREL_UNDEFINED,
- VREL_UNDEFINED, VREL_LT },
+ VREL_UNDEFINED, VREL_LT, VREL_OTHER },
// VREL_LE
{ VREL_LE, VREL_UNDEFINED, VREL_LT, VREL_LE, VREL_UNDEFINED, VREL_EQ,
- VREL_EQ, VREL_LT },
+ VREL_EQ, VREL_LT, VREL_OTHER },
// VREL_GT
{ VREL_GT, VREL_UNDEFINED, VREL_UNDEFINED, VREL_UNDEFINED, VREL_GT, VREL_GT,
- VREL_UNDEFINED, VREL_GT },
+ VREL_UNDEFINED, VREL_GT, VREL_OTHER },
// VREL_GE
{ VREL_GE, VREL_UNDEFINED, VREL_UNDEFINED, VREL_EQ, VREL_GT, VREL_GE,
- VREL_EQ, VREL_GT },
+ VREL_EQ, VREL_GT, VREL_OTHER },
// VREL_EQ
{ VREL_EQ, VREL_UNDEFINED, VREL_UNDEFINED, VREL_EQ, VREL_UNDEFINED, VREL_EQ,
- VREL_EQ, VREL_UNDEFINED },
+ VREL_EQ, VREL_UNDEFINED, VREL_OTHER },
// VREL_NE
{ VREL_NE, VREL_UNDEFINED, VREL_LT, VREL_LT, VREL_GT, VREL_GT,
- VREL_UNDEFINED, VREL_NE } };
+ VREL_UNDEFINED, VREL_NE, VREL_OTHER },
+// VREL_OTHER
+ { VREL_OTHER, VREL_UNDEFINED, VREL_OTHER, VREL_OTHER, VREL_OTHER,
+ VREL_OTHER, VREL_OTHER, VREL_OTHER, VREL_OTHER } };
// Intersect relation R1 with relation R2 and return the resulting relation.
@@ -113,28 +117,31 @@ relation_intersect (relation_kind r1, relation_kind r2)
relation_kind rr_union_table[VREL_LAST][VREL_LAST] = {
// VREL_VARYING
{ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING,
- VREL_VARYING, VREL_VARYING, VREL_VARYING },
+ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING },
// VREL_UNDEFINED
{ VREL_VARYING, VREL_UNDEFINED, VREL_LT, VREL_LE, VREL_GT, VREL_GE,
- VREL_EQ, VREL_NE },
+ VREL_EQ, VREL_NE, VREL_OTHER },
// VREL_LT
{ VREL_VARYING, VREL_LT, VREL_LT, VREL_LE, VREL_NE, VREL_VARYING, VREL_LE,
- VREL_NE },
+ VREL_NE, VREL_OTHER },
// VREL_LE
{ VREL_VARYING, VREL_LE, VREL_LE, VREL_LE, VREL_VARYING, VREL_VARYING,
- VREL_LE, VREL_VARYING },
+ VREL_LE, VREL_VARYING, VREL_OTHER },
// VREL_GT
{ VREL_VARYING, VREL_GT, VREL_NE, VREL_VARYING, VREL_GT, VREL_GE, VREL_GE,
- VREL_NE },
+ VREL_NE, VREL_OTHER },
// VREL_GE
{ VREL_VARYING, VREL_GE, VREL_VARYING, VREL_VARYING, VREL_GE, VREL_GE,
- VREL_GE, VREL_VARYING },
+ VREL_GE, VREL_VARYING, VREL_OTHER },
// VREL_EQ
{ VREL_VARYING, VREL_EQ, VREL_LE, VREL_LE, VREL_GE, VREL_GE, VREL_EQ,
- VREL_VARYING },
+ VREL_VARYING, VREL_OTHER },
// VREL_NE
{ VREL_VARYING, VREL_NE, VREL_NE, VREL_VARYING, VREL_NE, VREL_VARYING,
- VREL_VARYING, VREL_NE } };
+ VREL_VARYING, VREL_NE, VREL_OTHER },
+// VREL_OTHER
+ { VREL_VARYING, VREL_OTHER, VREL_OTHER, VREL_OTHER, VREL_OTHER,
+ VREL_OTHER, VREL_OTHER, VREL_OTHER, VREL_OTHER } };
// Union relation R1 with relation R2 and return the result.
@@ -151,28 +158,31 @@ relation_union (relation_kind r1, relation_kind r2)
relation_kind rr_transitive_table[VREL_LAST][VREL_LAST] = {
// VREL_VARYING
{ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING,
- VREL_VARYING, VREL_VARYING, VREL_VARYING },
+ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING },
// VREL_UNDEFINED
{ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING,
- VREL_VARYING, VREL_VARYING, VREL_VARYING },
+ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING },
// VREL_LT
{ VREL_VARYING, VREL_VARYING, VREL_LT, VREL_LT, VREL_VARYING, VREL_VARYING,
- VREL_LT, VREL_VARYING },
+ VREL_LT, VREL_VARYING, VREL_VARYING },
// VREL_LE
{ VREL_VARYING, VREL_VARYING, VREL_LT, VREL_LE, VREL_VARYING, VREL_VARYING,
- VREL_LE, VREL_VARYING },
+ VREL_LE, VREL_VARYING, VREL_VARYING },
// VREL_GT
{ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_GT, VREL_GT,
- VREL_GT, VREL_VARYING },
+ VREL_GT, VREL_VARYING, VREL_VARYING },
// VREL_GE
{ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_GT, VREL_GE,
- VREL_GE, VREL_VARYING },
+ VREL_GE, VREL_VARYING, VREL_VARYING },
// VREL_EQ
{ VREL_VARYING, VREL_VARYING, VREL_LT, VREL_LE, VREL_GT, VREL_GE, VREL_EQ,
- VREL_VARYING },
+ VREL_VARYING, VREL_VARYING },
// VREL_NE
{ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING,
- VREL_VARYING, VREL_VARYING, VREL_VARYING } };
+ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING },
+// VREL_OTHER
+ { VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING,
+ VREL_VARYING, VREL_VARYING, VREL_VARYING, VREL_VARYING } };
// Apply transitive operation between relation R1 and relation R2, and
// return the resulting relation, if any.
@@ -187,7 +197,7 @@ relation_transitive (relation_kind r1, relation_kind r2)
tree_code relation_to_code [VREL_LAST] = {
ERROR_MARK, ERROR_MARK, LT_EXPR, LE_EXPR, GT_EXPR, GE_EXPR, EQ_EXPR,
- NE_EXPR };
+ NE_EXPR, ERROR_MARK };
// This routine validates that a relation can be applied to a specific set of
// ranges. In particular, floating point x == x may not be true if the NaN bit
@@ -784,17 +794,25 @@ value_relation::negate ()
bool
value_relation::intersect (const value_relation &p)
{
- // Save previous value
- relation_kind old = related;
+ relation_kind k;
if (p.op1 () == op1 () && p.op2 () == op2 ())
- related = relation_intersect (kind (), p.kind ());
+ k = relation_intersect (kind (), p.kind ());
else if (p.op2 () == op1 () && p.op1 () == op2 ())
- related = relation_intersect (kind (), relation_swap (p.kind ()));
+ k = relation_intersect (kind (), relation_swap (p.kind ()));
else
return false;
- return old != related;
+ if (related == k)
+ return false;
+
+ // Floating point relations may include NANs, so don't produce UNDEFINED.
+ bool float_p = name1 && HONOR_NANS (TREE_TYPE (name1));
+ if (float_p && p.kind () != k && k == VREL_UNDEFINED)
+ k = VREL_OTHER;
+
+ related = k;
+ return true;
}
// Perform a union between 2 relations. *this ||= p.
@@ -802,17 +820,31 @@ value_relation::intersect (const value_relation &p)
bool
value_relation::union_ (const value_relation &p)
{
- // Save previous value
- relation_kind old = related;
+ relation_kind k;
if (p.op1 () == op1 () && p.op2 () == op2 ())
- related = relation_union (kind(), p.kind());
+ k = relation_union (kind (), p.kind ());
else if (p.op2 () == op1 () && p.op1 () == op2 ())
- related = relation_union (kind(), relation_swap (p.kind ()));
+ k = relation_union (kind (), relation_swap (p.kind ()));
else
return false;
- return old != related;
+ if (related == k)
+ return false;
+
+ // (x < y) || (x > y) produces x != y, but this is not true with floats.
+ // (x <= y) || (x >= y) produces VARYING, which is also not true for floats.
+ // As they cannot be properly represented, use VREL_OTHER.
+ bool float_p = name1 && HONOR_NANS (TREE_TYPE (name1));
+ if (float_p && p.kind () != k)
+ {
+ if (relation_lt_le_gt_ge_p (kind ())
+ || relation_lt_le_gt_ge_p (p.kind ()))
+ k = VREL_OTHER;
+ }
+
+ related = k;
+ return true;
}
// Identify and apply any transitive relations between REL
diff --git a/gcc/value-relation.h b/gcc/value-relation.h
index 354a0fd4130..42226f60552 100644
--- a/gcc/value-relation.h
+++ b/gcc/value-relation.h
@@ -70,6 +70,7 @@ typedef enum relation_kind_t
VREL_GE, // r1 >= r2
VREL_EQ, // r1 == r2
VREL_NE, // r1 != r2
+ VREL_OTHER, // unrepresentable floating point relation.
VREL_PE8, // 8 bit partial equivalency
VREL_PE16, // 16 bit partial equivalency
VREL_PE32, // 32 bit partial equivalency
