| 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 | expand |
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.
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.
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
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
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
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
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
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
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
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
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
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 >
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
On November 13, 2018 5:40:59 PM GMT+01:00, Aldy Hernandez <aldyh@redhat.com> 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
On 11/13/18 1:47 PM, Richard Biener wrote: > On November 13, 2018 5:40:59 PM GMT+01:00, Aldy Hernandez <aldyh@redhat.com> 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
* 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;