[PATCH 2/2] Add VREL_OTHER for FP unsupported relations.

[Andrew MacLeod]

On 1/25/23 06:15, Jakub Jelinek wrote:
> On Tue, Jan 24, 2023 at 10:57:12AM -0500, Andrew MacLeod wrote:
>> 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.
> In that case, I think the problem isn't in the intersection, which works
> correctly, but in wherever we (again, for HONOR_NANS (type) only) make the
> assumptions about the else branches.
> In the above case, the first blah has VREL_LE, that is correct,
> but the else of it (when the condition isn't true but is false)
> isn't VREL_GT, but VREL_UNGT (aka VREL_OTHER).
> So, for the HONOR_NANS (type) cases we need to adjust relation_negate
> callers.
> On trees, you can e.g. look at fold-const.cc (invert_tree_comparison),
> though that one for HONOR_NANS (type) && flag_trapping_math punts in most
> cases to preserve exceptions.  But you can see there the !honor_nans
> cases where it acts like relation_negate, and then the honor_nans cases,
> where VREL_*:
> VARYING UNDEFINED LT LE GT GE EQ NE LTGT ORDERED UNORDERED UNLT UNLE UNGT UNGE UNEQ
> negate to
> UNDEFINED VARYING UNGE UNGT UNLE UNLT NE EQ UNEQ UNORDERED ORDERED GE GT LE LT LTGT
> Or, in the world where VREL_OTHER is a wildcard for
> LTGT ORDERED UNORDERED UNLT UNLE UNGT UNGE UNEQ
> the less useful
> VARYING UNDEFINED LT LE GT GE EQ NE OTHER
> negates to
> UNDEFINED VARYING OTHER OTHER OTHER OTHER NE EQ OTHER
>
> But I'm afraid the above has VREL_OTHER for too many important cases,
> unlike intersect where it is for none unless VREL_OTHER is involved, or just
> a few ones for union.

Im not sure it is quite that bad.   Floating point ranges and range-ops 
does a pretty good job of tracking NANs in the ranges. They then utilize 
any available relation in addition to that. So within floating point 
processing, all our relations are assumed to possibly include NAN, then 
the range processing in range-ops decides which variant of the relation 
is applicable.  When Aldy and I went thru this exercise initally, we 
concluded we didnt need all the various forms of relations, that its was 
completely isolated within the frange implementation.. which is ideal.

The only place this is causing real "trouble" is within ranger where we 
pre-fold a stmt based on the relation, without it ever getting to 
rangeops.  Yes,we dont represent <> or ordered or unordered in the 
relation table, but those are really very specific floating point things 
and ranger doesnt need to be aware of them anyway I don't think.  theres 
not much it can do with them.

Merge points use union and cascading conditions use intersection.  The 
current fix will eliminate ranger from making any presumptions about 
these cases early.  Those are about the only things ranger itself uses 
relations for.. everything else is deferred to range-ops which 
understands all the things like ordered/unordered etc and it will still 
fold these expression properly when evaluated.

ie  without any relational representation of unordered and unordered, we 
still fold this properly because frange tracks it all:

   if (x_2(D) unord x_2(D))
     goto <bb 3>; [INV]
   else
     goto <bb 5>; [INV]

   <bb 3> :
   if (x_2(D) ord x_2(D))
     goto <bb 4>; [INV]
   else
     goto <bb 5>; [INV]

   <bb 4> :
   link_error ();

and value relations don't need to be involved.  I am currently unaware 
of anything with floating point relations we don't track properly in the 
end.


> So, I wonder if we just shouldn't do it properly immediately and instead of
> VREL_OTHER introduce those
> VREL_LTGT,	/* Less than or greater than.  */
> VREL_ORDERED,	/* Operands not NAN.  */
> VREL_UNORDERED, /* One or both operands NAN.  */
> VREL_UNLT,	/* Unordered or less than.  */
> VREL_UNLE,	/* Unordered or less than or equal.  */
> VREL_UNGT,	/* Unordered or greater than.  */
> VREL_UNGE,	/* Unordered or greater than or equal.  */
> VREL_UNEQ	/* Unordered or equal.  */
> In that case, rather than using relation_{negate,union,intersect} etc.
> either those functions should take an argument whether it is a HONOR_NANS
> (type) floating point or not and use two separate tables under the hood,
> or have two sets of routines and choose which one to use in the callers.
> I think one routine with an extra argument would be cleaner though.
>
> The above would grow the tables quite a bit (though, we could for the
> non-FP cases limit ourselves to a subset), but because all the VREL_*
> constants are enumerals with small integers values and the arrays
> should be (but aren't for some reason?) static just make the arrays
> contain unsigned char and do the casts to the enum in the
> relation_{negate,intersect,union} etc. wrappers.
> Also, I think the rr_*_table arrays should be const, we don't want to change
> them, right?  And a few other arrays too, like relation_to_code.
> BTW, with the above extra 8 codes we could use the ORDERED_EXPR etc. tree
> codes there.
>
I am very hesitant about implementing full relations for all these 
floating point specific things when
   a) I'm not convinced we need them and

   b) its a much more invasive change at the stage of the release.  
Every floating point range-op relational entry will have to be adjusted, 
and Im not sure of the ripple effects, especially since frange does a 
lot of its won things with the relational entries.

So far this fix is for ranger to avoid doing something its not suppose 
to.  Are we aware of any cases where we aren't handling a floating point 
relation fold that we should?   I would hate to complicate all the 
relation handling if we don't need to.  It may mean we need something 
else.. but its unclear to me exactly what we do minimally need at this 
point.. I was hoping to do that early in stage 1.

Aldy understand frange better than I do, maybe he can chip in about what 
we might or might not be missing without any of those other codes...

Andrew