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
