On Thu, Nov 8, 2018 at 4:27 PM Jeff Law <l...@redhat.com> wrote:
>
> On 11/8/18 8:14 AM, Richard Biener wrote:
> > 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 ();
> I hate this hunk of code. In fact, it's probably the biggest wart from
> the classification & simplification work last year.
>
> The problem is the local stack object can't be pushed onto the unwinding
> stack -- it'll be destroyed when we leave scope and we end up popping
> total junk later when we restore the range.
>
> You also have to be careful because of the bitmap sharing.
_And_ the abstracted "allocator" doesn't allow initialization. _And_
the ->set methods are private.
Bummer.
Stupid C++.
Richard.
>
> Jeff
