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.
> + // (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.
> --- 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).
Jakub
