Re: Problems in IPA passes
Hi Jeff,
On 31 October 2017 at 14:47, Jeff Law wrote:
> On 10/29/2017 03:54 PM, Kugan Vivekanandarajah wrote:
>> Hi Jeff,
>>
>> On 28 October 2017 at 18:28, Jeff Law 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.
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.
Thanks,
Kugan
Re: Problems in IPA passes
On October 31, 2017 4:47:06 AM GMT+01:00, Jeff Law wrote:
>On 10/29/2017 03:54 PM, Kugan Vivekanandarajah wrote:
>> Hi Jeff,
>>
>> On 28 October 2017 at 18:28, Jeff Law 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 :(
Equivalence shouldn't be used from the extract_range_from_* routines. Vrp_meet
probably uses them though. Factoring this function would be nice.
Richard.
Re: Problems in IPA passes
On October 31, 2017 10:49:51 AM GMT+01:00, Kugan Vivekanandarajah
wrote:
>Hi Jeff,
>
>On 31 October 2017 at 14:47, Jeff Law wrote:
>> On 10/29/2017 03:54 PM, Kugan Vivekanandarajah wrote:
>>> Hi Jeff,
>>>
>>> On 28 October 2017 at 18:28, Jeff Law 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.
>
>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.
At some point I wanted to isolate those functions not relying on global state
but I never came to do this. Those are the building blocks alternative range
propagation stuff like Andrew's could use.
With C++ using a namespace like wide_int does would work I guess.
And yes, equivalences are ugly... Ideally we'd have a base value-range class
without them used by IPA and VRP (non-early) would use a derived class.
Richard.
Richard.
>Thanks,
>Kugan
Create a new mirror
Hello, We would like to create a new mirror for GCC in Dronten, Netherlands. Can you let us know how to process? Regards, Martin
Re: Problems in IPA passes
On 10/31/2017 03:49 AM, Kugan Vivekanandarajah wrote:
> Hi Jeff,
>
> On 31 October 2017 at 14:47, Jeff Law wrote:
>> On 10/29/2017 03:54 PM, Kugan Vivekanandarajah wrote:
>>> Hi Jeff,
>>>
>>> On 28 October 2017 at 18:28, Jeff Law 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
Re: Problems in IPA passes
On 10/31/2017 04:36 AM, Richard Biener wrote:
> On October 31, 2017 10:49:51 AM GMT+01:00, Kugan Vivekanandarajah
> wrote:
>> Hi Jeff,
>>
>> On 31 October 2017 at 14:47, Jeff Law wrote:
>>> On 10/29/2017 03:54 PM, Kugan Vivekanandarajah wrote:
Hi Jeff,
On 28 October 2017 at 18:28, Jeff Law 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.
>>
>> 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.
>
> At some point I wanted to isolate those functions not relying on
> global state but I never came to do this. Those are the building
> blocks alternative range propagation stuff like Andrew's could use.
So in my tree it's no longer global state :-)
> And yes, equivalences are ugly... Ideally we'd have a base
> value-range class without them used by IPA and VRP (non-early) would
> use a derived class.
I'll look into that -- it's a fairly large wart in terms of global
access in vrp.
In my local tree I'm just passing around the vrp_bitmap_obstack right
now. Nobody's accessing it via a global anymore. So at least we know
what routines directly or indirectly want to touch vrp_bitmap_obstack.
Jeff
Re: Problems in IPA passes
On 10/31/2017 04:31 AM, Richard Biener wrote: > > Equivalence shouldn't be used from the extract_range_from_* routines. > Vrp_meet probably uses them though. Factoring this function would be nice. You'd think it shouldn't be needed :-) If we look at extract_range_from_binary_expr_1, you'll see it calls vrp_meet... Sigh. WHich means that everything that calls extract_range_from_binary_expr_1 needs the equivalences passed around. Similarly we often call set_value_range* which wants access to the bitmap obstack as well. Jeff
Re: Problems in IPA passes
On October 31, 2017 4:49:20 PM GMT+01:00, Jeff Law wrote:
>On 10/31/2017 04:36 AM, Richard Biener wrote:
>> On October 31, 2017 10:49:51 AM GMT+01:00, Kugan Vivekanandarajah
>> wrote:
>>> Hi Jeff,
>>>
>>> On 31 October 2017 at 14:47, Jeff Law wrote:
On 10/29/2017 03:54 PM, Kugan Vivekanandarajah wrote:
> Hi Jeff,
>
> On 28 October 2017 at 18:28, Jeff Law 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.
>>>
>>> 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.
>>
>> At some point I wanted to isolate those functions not relying on
>> global state but I never came to do this. Those are the building
>> blocks alternative range propagation stuff like Andrew's could use.
>So in my tree it's no longer global state :-)
>
>> And yes, equivalences are ugly... Ideally we'd have a base
>> value-range class without them used by IPA and VRP (non-early) would
>> use a derived class.
>I'll look into that -- it's a fairly large wart in terms of global
>access in vrp.
>
>In my local tree I'm just passing around the vrp_bitmap_obstack right
>now. Nobody's accessing it via a global anymore. So at least we know
>what routines directly or indirectly want to touch vrp_bitmap_obstack.
Maybe that's not necessary in most places given existing bitmaps know their
obstack? IIRC most places do sth to equivalences only if a range already
contains some.
Richard.
>
>Jeff
