On Mon, 12 Nov 2018, Aldy Hernandez wrote:
> On 11/12/18 7:12 AM, Richard Biener wrote:
> >
> > This mainly tries to rectify the workaround I put in place for ipa-cp.c
> > needing to build value_range instead of value_range_base for calling
> > extract_range_from_unary_expr.
> >
> > To make this easier I moved more set_* functions to methods.
> >
> > Then for some reason I chose to fix the rathole of equiv bitmap sharing
> > after finding at least one real bug
>
> By the way, I've seen that the equiv_add() calls in vr-values.c sometimes set
> equivalences for VARYING and UNDEFINED which in theory shouldn't happen. I've
> been too chicken to follow that hole.
>
> I think we should assert everywhere that we set the equivalences, that we're
> not talking about VARYING or UNDEFINED.
>
> > @@ -6168,37 +6172,30 @@ value_range::union_helper (value_range *vr0, const
> > value_range *vr1)
> > return;
> > }
> > - value_range saved (*vr0);
> > value_range_kind vr0type = vr0->kind ();
> > tree vr0min = vr0->min ();
> > tree vr0max = vr0->max ();
> > union_ranges (&vr0type, &vr0min, &vr0max,
> > vr1->kind (), vr1->min (), vr1->max ());
> > - *vr0 = value_range (vr0type, vr0min, vr0max);
> > - if (vr0->varying_p ())
> > + /* Work on a temporary so we can still use vr0 when union returns
> > varying. */
> > + value_range tem;
> > + tem.set_and_canonicalize (vr0type, vr0min, vr0max);
> > + if (tem.varying_p ())
>
> I'm not a big fan of the code duplication in the union chunks. You're adding
> more places that need to be kept in sync.
>
> I think value_range::union_ could be easily coded as:
>
> value_range_base::union_ (other);
> union_helper (this, other);
> if (flag_checking)
> check ();
>
> And have union_helper only deal with the equivalence stuff.
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).
> Call it
> union_equivs? You'd have to clear the equivalences if the range just became a
> varying/undefined, as both of those should in theory never have equivalences.
>
> Also, is there a reason why you implemented value_range_base::union_ but not
> the corresponding for intersect? I would guess it'd be needed sooner or
> later.
Laziness ;) But yes. It was mostly time constraints for hitting Stage1
with the major changes.
Richard.
