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