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
