record_ranges_from_incoming_edge: use value_range API for creating new range

Message ID 79800e67-85d6-be61-b105-65e945361bf9@redhat.com
State New
Headers show
Series
  • record_ranges_from_incoming_edge: use value_range API for creating new range
Related show

Commit Message

Aldy Hernandez Nov. 8, 2018, 12:17 p.m.
This one's rather obvious and does not depend on any get_range_info API 
change.

OK for trunk?

Comments

Richard Biener Nov. 8, 2018, 2:24 p.m. | #1
On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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().

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.
Richard Biener Nov. 8, 2018, 2:30 p.m. | #2
On Thu, Nov 8, 2018 at 3:24 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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().
>
> 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.
Aldy Hernandez Nov. 8, 2018, 2:30 p.m. | #3
On 11/8/18 9:24 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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.

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
Richard Biener Nov. 8, 2018, 2:49 p.m. | #4
On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 11/8/18 9:24 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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.

> 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
Jeff Law Nov. 8, 2018, 2:53 p.m. | #5
On 11/8/18 7:49 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>>
>>
>> On 11/8/18 9:24 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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.
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
Aldy Hernandez Nov. 8, 2018, 2:54 p.m. | #6
On 11/8/18 9:49 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>>
>>
>> On 11/8/18 9:24 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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?

> 
>> 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
Richard Biener Nov. 8, 2018, 2:56 p.m. | #7
On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 11/8/18 9:49 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >>
> >>
> >> On 11/8/18 9:24 AM, Richard Biener wrote:
> >>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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.

Richard.

>
> Aldy
Aldy Hernandez Nov. 8, 2018, 3 p.m. | #8
On 11/8/18 9:56 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>>
>>
>> On 11/8/18 9:49 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>> On 11/8/18 9:24 AM, Richard Biener wrote:
>>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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!

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

Aldy
Richard Biener Nov. 8, 2018, 3:14 p.m. | #9
On Thu, Nov 8, 2018 at 4:00 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 11/8/18 9:56 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >>
> >>
> >> On 11/8/18 9:49 AM, Richard Biener wrote:
> >>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 11/8/18 9:24 AM, Richard Biener wrote:
> >>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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
Jeff Law Nov. 8, 2018, 3:27 p.m. | #10
On 11/8/18 8:14 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 4:00 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>>
>>
>> On 11/8/18 9:56 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>> On 11/8/18 9:49 AM, Richard Biener wrote:
>>>>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/8/18 9:24 AM, Richard Biener wrote:
>>>>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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.

Jeff
Richard Biener Nov. 9, 2018, 11:30 a.m. | #11
On Thu, Nov 8, 2018 at 4:27 PM Jeff Law <law@redhat.com> wrote:
>
> On 11/8/18 8:14 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 4:00 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >>
> >>
> >> On 11/8/18 9:56 AM, Richard Biener wrote:
> >>> On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 11/8/18 9:49 AM, Richard Biener wrote:
> >>>>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 11/8/18 9:24 AM, Richard Biener wrote:
> >>>>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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
Aldy Hernandez Nov. 9, 2018, 11:56 a.m. | #12
Patches welcome!

On Fri, Nov 9, 2018, 12:30 Richard Biener <richard.guenther@gmail.com wrote:

> On Thu, Nov 8, 2018 at 4:27 PM Jeff Law <law@redhat.com> wrote:
> >
> > On 11/8/18 8:14 AM, Richard Biener wrote:
> > > On Thu, Nov 8, 2018 at 4:00 PM Aldy Hernandez <aldyh@redhat.com>
> wrote:
> > >>
> > >>
> > >>
> > >> On 11/8/18 9:56 AM, Richard Biener wrote:
> > >>> On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <aldyh@redhat.com>
> wrote:
> > >>>>
> > >>>>
> > >>>>
> > >>>> On 11/8/18 9:49 AM, Richard Biener wrote:
> > >>>>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <aldyh@redhat.com>
> wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>> On 11/8/18 9:24 AM, Richard Biener wrote:
> > >>>>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <aldyh@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
>

Patch

            * gimple-ssa-evrp-analyze.c
            (evrp_range_analyzer::record_ranges_from_incoming_edge): Use
            value_range API instead of piecing together ranges.

diff --git a/gcc/gimple-ssa-evrp-analyze.c b/gcc/gimple-ssa-evrp-analyze.c
index 3e5287b1b0b..ec39895cceb 100644
--- a/gcc/gimple-ssa-evrp-analyze.c
+++ b/gcc/gimple-ssa-evrp-analyze.c
@@ -207,7 +207,8 @@  evrp_range_analyzer::record_ranges_from_incoming_edge (basic_block bb)
 	         getting first [64, +INF] and then ~[0, 0] from
 		 conditions like (s & 0x3cc0) == 0).  */
 	      value_range *old_vr = get_value_range (vrs[i].first);
-	      value_range tem (old_vr->kind (), old_vr->min (), old_vr->max ());
+	      value_range tem = *old_vr;
+	      tem.equiv_clear ();
 	      tem.intersect (vrs[i].second);
 	      if (tem.ignore_equivs_equal_p (*old_vr))
 		continue;