[PATCH] More value_range API cleanup

[Richard Biener]

On Tue, 13 Nov 2018, Aldy Hernandez wrote:

> 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?

That was part of the previous (or previous previous) change.

> 
> > +/* 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.

The method is static now and given it doesn't modify VR0 now
but returns a copy that is better IMHO.

Richard.