record_ranges_from_incoming_edge: use value_range API for creating new range

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

record_ranges_from_incoming_edge: use value_range API for creating new range

Aldy Hernandez-2
This one's rather obvious and does not depend on any get_range_info API
change.

OK for trunk?

curr.patch (875 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Richard Biener-2
On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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().

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.

It's also two lines of code instead of one.

Richard.
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Richard Biener-2
On Thu, Nov 8, 2018 at 3:24 PM Richard Biener
<[hidden email]> wrote:

>
> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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().
>
> 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.
>
> It's also two lines of code instead of one.

And...  (making uses of operator= not link):

/space/rguenther/src/gcc-slpcost/gcc/ipa-prop.c:1781: undefined
reference to `value_range::operator=(value_range const&)'
/space/rguenther/src/gcc-slpcost/gcc/tree-ssa-threadedge.c:169:
undefined reference to `value_range::operator=(value_range const&)'
/space/rguenther/src/gcc-slpcost/gcc/tree-vrp.c:230: undefined
reference to `value_range::operator=(value_range const&)'
/space/rguenther/src/gcc-slpcost/gcc/tree-vrp.c:237: undefined
reference to `value_range::operator=(value_range const&)'
/space/rguenther/src/gcc-slpcost/gcc/tree-vrp.c:562: undefined
reference to `value_range::operator=(value_range const&)'
tree-vrp.o:/space/rguenther/src/gcc-slpcost/gcc/tree-vrp.c:1163: more
undefined references to `value_range::operator=(value_range const&)'
follow

can you investiage all those for the same error?

Since we need to do deep copying for the equiv bitmap I think
operator=() should be
not implemented:

diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index c251329a195..ad4b0cd621b 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -45,6 +45,7 @@ class GTY((for_user)) value_range
   void update (value_range_kind, tree, tree);
   bool operator== (const value_range &) const;
   bool operator!= (const value_range &) const;
+  value_range& operator=(const value_range &);
   void intersect (const value_range *);
   void union_ (const value_range *);

likewise for copy construction.

Richard.

> Richard.
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Aldy Hernandez-2
In reply to this post by Richard Biener-2


On 11/8/18 9:24 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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.

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.

Aldy
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Richard Biener-2
On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <[hidden email]> wrote:

>
>
>
> On 11/8/18 9:24 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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.

> 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.

Richard.

> Aldy
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Jeff Law
On 11/8/18 7:49 AM, Richard Biener wrote:

> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <[hidden email]> wrote:
>>
>>
>>
>> On 11/8/18 9:24 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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.
Agreed.  The existence of the bitmaps to me indicates these objects
aren't really good candidates for operator overloads.

>
>> 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.
Most likely yes based on my experiences last year with teasing bits of
this stuff apart.

jeff
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Aldy Hernandez-2
In reply to this post by Richard Biener-2


On 11/8/18 9:49 AM, Richard Biener wrote:

> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <[hidden email]> wrote:
>>
>>
>>
>> On 11/8/18 9:24 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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?

>
>> 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?

Aldy
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Richard Biener-2
On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <[hidden email]> wrote:

>
>
>
> On 11/8/18 9:49 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <[hidden email]> wrote:
> >>
> >>
> >>
> >> On 11/8/18 9:24 AM, Richard Biener wrote:
> >>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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.

Richard.

>
> Aldy
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Aldy Hernandez-2


On 11/8/18 9:56 AM, Richard Biener wrote:

> On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <[hidden email]> wrote:
>>
>>
>>
>> On 11/8/18 9:49 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <[hidden email]> wrote:
>>>>
>>>>
>>>>
>>>> On 11/8/18 9:24 AM, Richard Biener wrote:
>>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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!

In the meantime I could provide equal_p(bool ignore_equivs) and perhaps
copy(bool ignore_equivs), while disabling assignment and comparison
operators.

Aldy
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Richard Biener-2
On Thu, Nov 8, 2018 at 4:00 PM Aldy Hernandez <[hidden email]> wrote:

>
>
>
> On 11/8/18 9:56 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <[hidden email]> wrote:
> >>
> >>
> >>
> >> On 11/8/18 9:49 AM, Richard Biener wrote:
> >>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <[hidden email]> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 11/8/18 9:24 AM, Richard Biener wrote:
> >>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Jeff Law
On 11/8/18 8:14 AM, Richard Biener wrote:

> On Thu, Nov 8, 2018 at 4:00 PM Aldy Hernandez <[hidden email]> wrote:
>>
>>
>>
>> On 11/8/18 9:56 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <[hidden email]> wrote:
>>>>
>>>>
>>>>
>>>> On 11/8/18 9:49 AM, Richard Biener wrote:
>>>>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/8/18 9:24 AM, Richard Biener wrote:
>>>>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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.

Jeff
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Richard Biener-2
On Thu, Nov 8, 2018 at 4:27 PM Jeff Law <[hidden email]> wrote:

>
> On 11/8/18 8:14 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 4:00 PM Aldy Hernandez <[hidden email]> wrote:
> >>
> >>
> >>
> >> On 11/8/18 9:56 AM, Richard Biener wrote:
> >>> On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <[hidden email]> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 11/8/18 9:49 AM, Richard Biener wrote:
> >>>>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <[hidden email]> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 11/8/18 9:24 AM, Richard Biener wrote:
> >>>>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Aldy Hernandez-2
Patches welcome!

On Fri, Nov 9, 2018, 12:30 Richard Biener <[hidden email] wrote:

> On Thu, Nov 8, 2018 at 4:27 PM Jeff Law <[hidden email]> wrote:
> >
> > On 11/8/18 8:14 AM, Richard Biener wrote:
> > > On Thu, Nov 8, 2018 at 4:00 PM Aldy Hernandez <[hidden email]>
> wrote:
> > >>
> > >>
> > >>
> > >> On 11/8/18 9:56 AM, Richard Biener wrote:
> > >>> On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <[hidden email]>
> wrote:
> > >>>>
> > >>>>
> > >>>>
> > >>>> On 11/8/18 9:49 AM, Richard Biener wrote:
> > >>>>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <[hidden email]>
> wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>> On 11/8/18 9:24 AM, Richard Biener wrote:
> > >>>>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <[hidden email]>
> 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
>
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Aldy Hernandez-2
In reply to this post by Richard Biener-2
With your cleanups, the main raison d'etre for my patch goes away, but
here is the promised removal of ignore_equivs_equal_p.

I think the == operator is a bit confusing, and equality intent should
be clearly specified.  I am providing the following for the derived
class (with no hidden default arguments):

        bool equal_p (const value_range &, bool ignore_equivs) const;

and providing the following for the base class:

        bool equal_p (const value_range_base &) const;

I am also removing access to both the == and the != operators.  It
should now be clear from the code whether the equivalence bitmap is
being taken into account or not.

What do you think?

Aldy
Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Richard Biener-2
On November 13, 2018 5:40:59 PM GMT+01:00, Aldy Hernandez <[hidden email]> wrote:

>With your cleanups, the main raison d'etre for my patch goes away, but
>here is the promised removal of ignore_equivs_equal_p.
>
>I think the == operator is a bit confusing, and equality intent should
>be clearly specified.  I am providing the following for the derived
>class (with no hidden default arguments):
>
> bool equal_p (const value_range &, bool ignore_equivs) const;
>
>and providing the following for the base class:
>
> bool equal_p (const value_range_base &) const;
>
>I am also removing access to both the == and the != operators.  It
>should now be clear from the code whether the equivalence bitmap is
>being taken into account or not.
>
>What do you think?

Sounds good.

Richard.

>Aldy

Reply | Threaded
Open this post in threaded view
|

Re: record_ranges_from_incoming_edge: use value_range API for creating new range

Aldy Hernandez-2
On 11/13/18 1:47 PM, Richard Biener wrote:

> On November 13, 2018 5:40:59 PM GMT+01:00, Aldy Hernandez <[hidden email]> wrote:
>> With your cleanups, the main raison d'etre for my patch goes away, but
>> here is the promised removal of ignore_equivs_equal_p.
>>
>> I think the == operator is a bit confusing, and equality intent should
>> be clearly specified.  I am providing the following for the derived
>> class (with no hidden default arguments):
>>
>> bool equal_p (const value_range &, bool ignore_equivs) const;
>>
>> and providing the following for the base class:
>>
>> bool equal_p (const value_range_base &) const;
>>
>> I am also removing access to both the == and the != operators.  It
>> should now be clear from the code whether the equivalence bitmap is
>> being taken into account or not.
>>
>> What do you think?
>
> Sounds good.

Committed to trunk.

Thanks.
Aldy