On Thu, Nov 8, 2018 at 4:00 PM Aldy Hernandez <al...@redhat.com> wrote:
>
>
>
> On 11/8/18 9:56 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <al...@redhat.com> wrote:
> >>
> >>
> >>
> >> On 11/8/18 9:49 AM, Richard Biener wrote:
> >>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <al...@redhat.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 11/8/18 9:24 AM, Richard Biener wrote:
> >>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <al...@redhat.com> wrote:
> >>>>>>
> >>>>>> This one's rather obvious and does not depend on any get_range_info API
> >>>>>> change.
> >>>>>>
> >>>>>> OK for trunk?
> >>>>>
> >>>>> Hmm, no - that's broken. IIRC m_equiv are shared bitmaps if you
> >>>>> do tem = *old_vr so you modify it in place with equiv_clear().
> >>>>
> >>>> Good point.
> >>>>
> >>>>>
> >>>>> Thus, operator= should be really deleted or mapped to value_range::set()
> >>>>> in which case tem = *old_vr would do useless bitmap allocation and
> >>>>> copying that you then clear.
> >>>>
> >>>> Actually, I was thinking that perhaps the assignment and equality
> >>>> operators should disregard the equivalence bitmap. In this case, copy
> >>>> everything EXCEPT the bitmap and set the resulting equivalence bitmap to
> >>>> NULL.
> >>>
> >>> I think that's equally confusing.
> >>
> >> I don't think so. When you ask whether range A and range B are equal,
> >> you're almost always interested in the range itself, not the equivalence
> >> table behind it.
> >>
> >> We could also get rid of the == operator and just provide:
> >>
> >> bool equal_p(bool ignore_equivs);
> >>
> >> How does this sound?
> >
> > Good.
> >
> >>>
> >>>> It's also annoying to use ::ignore_equivs_equal_p(). Since that seems
> >>>> to be the predominant way of comparing ranges, perhaps it should be the
> >>>> default.
> >>>
> >>> I think a good approach would be to isolate m_equiv more because it is
> >>> really an implementation detail of the propagator. Thus, make
> >>>
> >>> class value_range_with_equiv : public value_range
> >>> {
> >>> ... all the equiv stuff..
> >>> }
> >>>
> >>> make the lattice of type value_range_with_equiv and see what tickles
> >>> down.
> >>>
> >>> value_range_with_equiv wouldn't implement copy and assignment
> >>> (too expensive) and value_range can do with the trivial implementation.
> >>>
> >>> And most consumers/workers can just work on the equiv-less variants.
> >>
> >> I like this. Unfortunately, not feasible for this cycle (for me
> >> anyhow-- patches welcome though :)). How about equal_p() as described
> >> above?
> >
> > Works for me but you still need to sort out the copying, so if you think
> > splitting is not feasible (I'll give it a try) then please disable
> > assingment
> > and copy operators and fixup code.
>
> Are you saying you'll try implementing value_range_with_equiv :
> value_range? That would be of great help!
Yes, I'll experiment with this. Meanwhile noticed bogus
/* We're going to need to unwind this range. We can
not use VR as that's a stack object. We have to allocate
a new range and push the old range onto the stack. We
also have to be very careful about sharing the underlying
bitmaps. Ugh. */
value_range *new_vr = vr_values->allocate_value_range ();
*new_vr = vr;
new_vr->equiv_clear ();
...
> In the meantime I could provide equal_p(bool ignore_equivs) and perhaps
> copy(bool ignore_equivs), while disabling assignment and comparison
> operators.
Yes, that sounds good.
Richard.
> Aldy
