[PATCH] More value_range API cleanup

[Aldy Hernandez]

On 11/13/18 3:07 AM, Richard Biener wrote:
> On Tue, 13 Nov 2018, Aldy Hernandez wrote:
> 
>>
>>>> The tricky part starts in the prologue for
>>>>
>>>>     if (vr0->undefined_p ())
>>>>       {
>>>>         vr0->deep_copy (vr1);
>>>>         return;
>>>>       }
>>>>
>>>> but yes, we probably can factor out a bit more common code
>>>> here.  I'll see to followup with more minor cleanups this
>>>> week (noticed a few details myself).
>>>
>>> Like this?  (untested)
>>
>> I would inline value_range_base::union_helper into value_range_base::union_,
>> and remove all the undefined/varying/etc stuff from value_range::union_.
>>
>> If should work because upon return from value_range_base::union_, in the
>> this->undefined_p case, the base class will copy everything but the
>> equivalences.  Then the derived union_ only has to nuke the equivalences if
>> this->undefined or this->varying, and the equivalences' IOR just works.
>>
>> For instance, in the case where other->undefined_p, there shouldn't be
>> anything in the equivalences so the IOR won't copy anything to this as
>> expected.  Similarly for this->varying_p.
>>
>> In the case of other->varying, this will already been set to varying so
>> neither this nor other should have anything in their equivalence fields, so
>> the IOR won't do anything.
>>
>> I think I covered all of them...the bitmap math should just work.  What do you
>> think?
> 
> I think the only case that will not work is the case when this->undefined
> (when we need the deep copy).  Because we'll not get the bitmap from
> other in that case.  So I've settled with the thing below (just
> special-casing that very case)

Ah, good point.

> 
>> Finally, as I've hinted before, I think we need to be careful that any time we
>> change state to VARYING / UNDEFINED from a base method, that the derived class
>> is in a sane state (there are no equivalences set per the API contract).  This
>> was not originally enforced in VRP, and I wouldn't be surprised if there are
>> dragons if we enforce honesty.  I suppose, since we have an API, we could
>> enforce this lazily: any time equiv() is called, clear the equivalences or
>> return NULL if it's varying or undefined?  Just a thought.
> 
> I have updated ->update () to adjust equiv when we update to VARYING
> or UNDEFINED.

Excellent idea.  I don't see that part in your patch though?


> +/* Helper for meet operation for value ranges.  Given two value ranges VR0 and
> +   VR1, return a range that contains both VR0 and VR1.  This may not be the
> +   smallest possible such range.  */
> +
> +value_range_base
> +value_range_base::union_helper (const value_range_base *vr0,
> +				const value_range_base *vr1)
> +{

I know this was my fault, but would you mind removing vr0 from 
union_helper?  Perhaps something like this:

value_range_base::union_helper (const value_range_base *other)

I think it'll be cleaner and more consistent this way.

Thanks.
Aldy