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.