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
