On 11/8/18 8:14 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 4:00 PM Aldy Hernandez <al...@redhat.com> wrote:
>>
>>
>>
>> On 11/8/18 9:56 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 3:54 PM Aldy Hernandez <al...@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>> On 11/8/18 9:49 AM, Richard Biener wrote:
>>>>> On Thu, Nov 8, 2018 at 3:31 PM Aldy Hernandez <al...@redhat.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/8/18 9:24 AM, Richard Biener wrote:
>>>>>>> On Thu, Nov 8, 2018 at 1:17 PM Aldy Hernandez <al...@redhat.com> wrote:
>>>>>>>>
>>>>>>>> This one's rather obvious and does not depend on any get_range_info API
>>>>>>>> change.
>>>>>>>>
>>>>>>>> OK for trunk?
>>>>>>>
>>>>>>> Hmm, no - that's broken. IIRC m_equiv are shared bitmaps if you
>>>>>>> do tem = *old_vr so you modify it in place with equiv_clear().
>>>>>>
>>>>>> Good point.
>>>>>>
>>>>>>>
>>>>>>> Thus, operator= should be really deleted or mapped to value_range::set()
>>>>>>> in which case tem = *old_vr would do useless bitmap allocation and
>>>>>>> copying that you then clear.
>>>>>>
>>>>>> Actually, I was thinking that perhaps the assignment and equality
>>>>>> operators should disregard the equivalence bitmap. In this case, copy
>>>>>> everything EXCEPT the bitmap and set the resulting equivalence bitmap to
>>>>>> NULL.
>>>>>
>>>>> I think that's equally confusing.
>>>>
>>>> I don't think so. When you ask whether range A and range B are equal,
>>>> you're almost always interested in the range itself, not the equivalence
>>>> table behind it.
>>>>
>>>> We could also get rid of the == operator and just provide:
>>>>
>>>> bool equal_p(bool ignore_equivs);
>>>>
>>>> How does this sound?
>>>
>>> Good.
>>>
>>>>>
>>>>>> It's also annoying to use ::ignore_equivs_equal_p(). Since that seems
>>>>>> to be the predominant way of comparing ranges, perhaps it should be the
>>>>>> default.
>>>>>
>>>>> I think a good approach would be to isolate m_equiv more because it is
>>>>> really an implementation detail of the propagator. Thus, make
>>>>>
>>>>> class value_range_with_equiv : public value_range
>>>>> {
>>>>> ... all the equiv stuff..
>>>>> }
>>>>>
>>>>> make the lattice of type value_range_with_equiv and see what tickles
>>>>> down.
>>>>>
>>>>> value_range_with_equiv wouldn't implement copy and assignment
>>>>> (too expensive) and value_range can do with the trivial implementation.
>>>>>
>>>>> And most consumers/workers can just work on the equiv-less variants.
>>>>
>>>> I like this. Unfortunately, not feasible for this cycle (for me
>>>> anyhow-- patches welcome though :)). How about equal_p() as described
>>>> above?
>>>
>>> Works for me but you still need to sort out the copying, so if you think
>>> splitting is not feasible (I'll give it a try) then please disable
>>> assingment
>>> and copy operators and fixup code.
>>
>> Are you saying you'll try implementing value_range_with_equiv :
>> value_range? That would be of great help!
>
> Yes, I'll experiment with this. Meanwhile noticed bogus
>
> /* We're going to need to unwind this range. We can
> not use VR as that's a stack object. We have to allocate
> a new range and push the old range onto the stack. We
> also have to be very careful about sharing the underlying
> bitmaps. Ugh. */
> value_range *new_vr = vr_values->allocate_value_range ();
> *new_vr = vr;
> new_vr->equiv_clear ();
I hate this hunk of code. In fact, it's probably the biggest wart from
the classification & simplification work last year.
The problem is the local stack object can't be pushed onto the unwinding
stack -- it'll be destroyed when we leave scope and we end up popping
total junk later when we restore the range.
You also have to be careful because of the bitmap sharing.
Jeff
