On 10/2/19 9:36 AM, Richard Sandiford wrote:
Andrew MacLeod <amacl...@redhat.com> writes:
On 10/2/19 6:52 AM, Richard Biener wrote:
+
+/* Return the inverse of a range. */
+
+void
+value_range_base::invert ()
+{
+ if (undefined_p ())
+ return;
+ if (varying_p ())
+ set_undefined ();
+ else if (m_kind == VR_RANGE)
+ m_kind = VR_ANTI_RANGE;
+ else if (m_kind == VR_ANTI_RANGE)
+ m_kind = VR_RANGE;
+ else
+ gcc_unreachable ();
+}
I don't think this is right for varying_p. ISTM that if something is
VR_VARYING that inverting it is still VR_VARYING. VR_UNDEFINED seems
particularly bad given its optimistic treatment elsewhere.
VR_VARYING isn't a range, it's a lattice state (likewise for VR_UNDEFINED).
It doesn't make sense to invert a lattice state. How you treat
VR_VARYING/VR_UNDEFINED
depends on context and so depends what 'invert' would do. I suggest to assert
that varying/undefined is never inverted.
True for a lattice state, not true for a range in the new context of the
ranger where
a) varying == range for type and
b) undefined == unreachable
This is a carry over from really old code where we only got part of it
fixed right a while ago.
invert ( varying ) == varying because we still know nothing about it,
its still range for type.
invert (undefined) == undefined because undefined is unreachable
which is viral.
So indeed, varying should return varying... So If its undefined or
varying, we should just return from the invert call. ie, not do anything
to the range. in the case of a lattice state, doing nothing to it
should not be harmful. I also expect it will never get called for a
pure lattice state since its only invoked from range-ops, at which point
we only are dealing with the range it represents.
I took a look and this bug hasn't been triggered because its only used
in a couple of places.
1) op1_range for EQ_EXPR and NE_EXPR when we have a true OR false
constant condition in both the LHS and OP2 position, it sometimes
inverts it via this call.. so its only when there is a specific boolean
range of [TRUE,TRUE] or [FALSE,FALSE]. when any range is undefined
or varying in those routines, theres a different path for the result
2) fold() for logical NOT, which also has a preliminary check for
varying or undefined and does nothing in those cases ie, returns the
existing value. IN fact, you can probably remove the special casing in
logical_not with this fix, which is indicative that it is correct.
IMO that makes invert a bit of a dangerous operation. E.g. for
ranges of unsigned bytes:
invert ({}) = invert(UNDEFINED) = UNDEFINED = {}
invert ([255, 255]) = ~[255, 255] = [0, 254]
...
invert ([3, 255]) = ~[3, 255] = [0, 2]
invert ([2, 255]) = ~[2, 255] = [0, 1]
invert ([1, 255]) = ~[1, 255] = [0, 0]
invert ([0, 255]) = invert(VARYING) = VARYING = [0, 255]
where there's no continuity at the extremes. Maybe it would be
better to open-code it in those two places instead?
Richard
Im not sure that the continuity is important since ranges are a little
bit odd at the edges anyway :-)
however, I will take the point that perhaps invert () has potentially
different meanings at the edges depending no how you look at it.
Ultimately that is why we ended up getting it wrong in the first place...
So, I audited all the current uses of invert() (it is not commonly used
either) , and we already special case the varying or undefined behaviour
when its appropriate before invert is invoked. So I think we can
reduce everyones concern about these edge cases by simply doing as you
guys suggest and gcc_assert() that the ranges are not undefined or
varying for invert().
Andrew
