Patches welcome!
On Fri, Nov 9, 2018, 12:30 Richard Biener <richard.guent...@gmail.com wrote:
> 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
>
