On 10/31/2017 03:49 AM, Kugan Vivekanandarajah wrote:
> Hi Jeff,
>
> On 31 October 2017 at 14:47, Jeff Law <l...@redhat.com> wrote:
>> On 10/29/2017 03:54 PM, Kugan Vivekanandarajah wrote:
>>> Hi Jeff,
>>>
>>> On 28 October 2017 at 18:28, Jeff Law <l...@redhat.com> wrote:
>>>>
>>>> Jan,
>>>>
>>>> What's the purpose behind calling vrp_meet and
>>>> extract_range_from_unary_expr from within the IPA passes?
>>>
>>> This is used such that when we have an argument to a function and this
>>> for which we know the VR and this intern is passed as a parameter to
>>> another. For example:
>>>
>>> void foo (int i)
>>> {
>>> ...
>>> bar (unary_op (i))
>>> ...
>>> }
>>>
>>> This is mainly to share what is done in tree-vrp.
>> Presumably you never have equivalences or anything like that, which
>> probably helps with not touching vrp_bitmap_obstack which isn't
>> initialized when you run the IPA bits.
>>
>>>>
>>>> AFAICT that is not safe to do. Various paths through those routines
>>>> will access static objects within tree-vrp.c which may not be
>>>> initialized when IPA runs (vrp_equiv_obstack, vr_value).
>>>
>>> IPA-VRP does not track equivalence and vr_value is not used.
>> But there's no enforcement and I'd be hard pressed to believe that all
>> the paths through the routines you use in tree-vrp aren't going to touch
>> vr_value, or vrp_bitmap_obstack. vrp_bitmap_obstack turns out to be
>> incredibly tangled into the implementations within tree-vrp.c :(
>>
>
> I looked into the usage and it does seem to be not using vr_value
> unless I am missing something. There are two overloaded functions
> here:
>
> extract_range_from_unary_expr (value_range *vr,
> enum tree_code code, tree type,
> value_range *vr0_, tree op0_type)
> is safe as this works with value_range and does not use
> get_value_range to access vr_value.
Agreed that it doesn't access vr_value. I think I got somewhat confused
by the overload and I hadn't worked through
extract_range_from_binary_expr_1 to see if needed vr_values or just the
vrp_bitmap_obstack (it just needs the latter). Once I narrowed what
extract_range_from_binary_expr_1 needed it was pretty clear.
But we still need access to vrp_bitmap_obstack via the calls to
copy_value_range and via the call to extract_range_from_binary_expr_1.
Now in the case of the calls from IPA we may not have equivalences
attached to the vr_range so that *may* be OK in practice, but this is
something that also needs some cleaning up to avoid nasty runtime
surprises in the future.
>
> extract_range_from_unary_expr (value_range *vr, enum tree_code code,
> tree type, tree op0)
> This is not safe as this takes tree as an argument and gets
> value_range by calling get_value_range. May be we should change the
> names to reflect this.
Well, what I suspect will happen is the former routine will continue to
be a free function while the latter will likely move into the vr_values
class. That's what I've got here and what allows me to quickly see
where the violations are.
The big benefit is it gets much harder to mess things up and we don't
have to do a deep manual inspection. A function that needs vr_values
will be within the class or perhaps have the class instance passed in.
It becomes very clear at compile time what does and does not need
vr_values.
Furthermore, if someone were to mess things up (say by trying to call
vrp_visit_assignment_or_call from the IPA passes), they'll see at
compile time that they don't have a class instance for vr_values rather
than seeing some kind of weird runtime failure.
Jeff
