Re: [PATCH, Pointer Bounds Checker 28/x] IPA CP
On 06/17/14 07:41, Martin Jambor wrote: Hi, On Wed, Jun 11, 2014 at 05:47:36PM +0400, Ilya Enkovich wrote: Here is fixed verison. I'm fine with the ipa-cp hunks but I cannot approve them, Honza is the right person to ask. I'll step in and say these bits are fine :-) Thanks for the reviews Martin. Ilya, please hold off installing until all the patches are approved. We're obviously trying to keep up with them as they come in. jeff
Re: [PATCH, Pointer Bounds Checker 28/x] IPA CP
Hi,
On Wed, Jun 11, 2014 at 05:47:36PM +0400, Ilya Enkovich wrote:
>
> Here is fixed verison.
I'm fine with the ipa-cp hunks but I cannot approve them, Honza is the
right person to ask.
Thanks,
Martin
>
> Thanks,
> Ilya
> --
> gcc/
>
> 2014-06-11 Ilya Enkovich
>
> * cgraph.h (cgraph_local_p): New.
> * ipa-cp.c (initialize_node_lattices): Use cgraph_local_p
> to handle instrumentation clones properly.
> (propagate_constants_accross_call): Do not propagate
> through instrumentation thunks.
>
>
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index 5e702a7..b225ebe 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -1556,4 +1556,17 @@ symtab_in_same_comdat_p (symtab_node *one, symtab_node
> *two)
> {
>return DECL_COMDAT_GROUP (one->decl) == DECL_COMDAT_GROUP (two->decl);
> }
> +
> +/* Return true if NODE is local. Instrumentation clones are counted as local
> + only when originla function is local. */
> +
> +static inline bool
> +cgraph_local_p (cgraph_node *node)
> +{
> + if (!node->instrumentation_clone || !node->instrumented_version)
> +return node->local.local;
> +
> + return node->local.local && node->instrumented_version->local.local;
> +}
> +
> #endif /* GCC_CGRAPH_H */
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 689378a..4318789 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -699,7 +699,7 @@ initialize_node_lattices (struct cgraph_node *node)
>int i;
>
>gcc_checking_assert (cgraph_function_with_gimple_body_p (node));
> - if (!node->local.local)
> + if (!cgraph_local_p (node))
> {
>/* When cloning is allowed, we can assume that externally visible
>functions are not called. We will compensate this by cloning
> @@ -1434,6 +1434,24 @@ propagate_constants_accross_call (struct cgraph_edge
> *cs)
>if (parms_count == 0)
> return false;
>
> + /* No propagation through instrumentation thunks is available yet.
> + It should be possible with proper mapping of call args and
> + instrumented callee params in the propagation loop below. But
> + this case mostly occurs when legacy code calls instrumented code
> + and it is not a primary target for optimizations.
> + We detect instrumentation thunks in aliases and thunks chain by
> + checking instrumentation_clone flag for chain source and target.
> + Going through instrumentation thunks we always have it changed
> + from 0 to 1 and all other nodes do not change it. */
> + if (!cs->callee->instrumentation_clone
> + && callee->instrumentation_clone)
> +{
> + for (i = 0; i < parms_count; i++)
> + ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
> + i));
> + return ret;
> +}
> +
>/* If this call goes through a thunk we must not propagate to the first
> (0th)
> parameter. However, we might need to uncover a thunk from below a
> series
> of aliases first. */
Re: [PATCH, Pointer Bounds Checker 28/x] IPA CP
On 11 Jun 15:07, Ilya Enkovich wrote:
> 2014-06-11 13:45 GMT+04:00 Martin Jambor :
> > Hi,
> >
> > On Wed, Jun 11, 2014 at 12:24:57PM +0400, Ilya Enkovich wrote:
> >> Hi,
> >>
> >> This patch fixes IPA CP pass to handle instrumented code correctly.
> >>
> >> Bootstrapped and tested on linux-x86_64.
> >>
> >> Thanks,
> >> Ilya
> >> --
> >> gcc/
> >>
> >> 2014-06-11 Ilya Enkovich
> >>
> >> * ipa-cp.c (initialize_node_lattices): Check original
> >> version locality for instrumentation clones.
> >> (propagate_constants_accross_call): Do not propagate
> >> through instrumentation thunks.
> >>
> >>
> >> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> >> index 689378a..683b9f0 100644
> >> --- a/gcc/ipa-cp.c
> >> +++ b/gcc/ipa-cp.c
> >> @@ -699,7 +699,10 @@ initialize_node_lattices (struct cgraph_node *node)
> >>int i;
> >>
> >>gcc_checking_assert (cgraph_function_with_gimple_body_p (node));
> >> - if (!node->local.local)
> >> + if (!node->local.local
> >> + || (node->instrumentation_clone
> >> + && node->instrumented_version
> >> + && !node->instrumented_version->local.local))
> >
> > This looks quite convoluted, can you please put the test into a new
> > predicate in cgraph.c? I assume you had to change other tests of the
> > local flag in a similar fashion anyway.
>
> You are right. Would cgraph_node_local_p be OK?
>
> >
> >> {
> >>/* When cloning is allowed, we can assume that externally visible
> >>functions are not called. We will compensate this by cloning
> >> @@ -1440,7 +1443,8 @@ propagate_constants_accross_call (struct cgraph_edge
> >> *cs)
> >>alias_or_thunk = cs->callee;
> >>while (alias_or_thunk->alias)
> >> alias_or_thunk = cgraph_alias_target (alias_or_thunk);
> >> - if (alias_or_thunk->thunk.thunk_p)
> >> + if (alias_or_thunk->thunk.thunk_p
> >> + && !alias_or_thunk->thunk.add_pointer_bounds_args)
> >
> > so there are thunks that do not change the first argument and so we do
> > want to propagate to/through it?
>
> Yes. Thunks marked as add_pointer_bounds_args do not change arguments,
> but add default pointer bounds for all pointer arguments.
>
> >> {
> >>ret |= set_all_contains_variable (ipa_get_parm_lattices
> >> (callee_info,
> >> 0));
> >> @@ -1449,6 +1453,20 @@ propagate_constants_accross_call (struct
> >> cgraph_edge *cs)
> >>else
> >> i = 0;
> >>
> >> + /* No propagation through instrumentation thunks is available yet.
> >> + It should be possible with proper mapping of call args and
> >> + instrumented callee params in the propagation loop below. But
> >> + this case mostly occurs when legacy code calls instrumented code
> >> + and it is not a primary target for optimizations. */
> >> + if (!alias_or_thunk->instrumentation_clone
> >> + && callee->instrumentation_clone)
> >> +{
> >> + for (; i < parms_count; i++)
> >> + ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
> >> + i));
> >> + return ret;
> >> +}
> >> +
> >
> > and these thunks are different from those marked as
> > thunk.add_pointer_bounds_args? If they are not, the previous hunk is
> > redundant.
>
> This check covers more cases. It catches all chains of aliases and
> thunks where thunk.add_pointer_bounds_args is met. It does not mean
> the first met thunk has thunk.add_pointer_bounds_args (as is in
> previous check). I suppose you are right about previous hunk. It
> would be better to make wider check first and make exit earlier. Will
> fix it.
>
> >
> > My apologies for not looking at the patches introducing all the new
> > cgraph_node fields but it is quite difficult to figure out what the
> > new tests actually mean (that is why I'd really prefer predicates with
> > more explanatory names).
>
> New predicates are good but do we need them for single usage? Locality
> check are used during output and new predicate would be nice for it.
> But there is no another thunk.add_pointer_bounds_args chain check used
> somewhere else. Will try to make more explanatory comment for it for
> now.
>
> Thanks,
> Ilya
>
> >
> > Thanks,
> >
> > Martin
Here is fixed verison.
Thanks,
Ilya
--
gcc/
2014-06-11 Ilya Enkovich
* cgraph.h (cgraph_local_p): New.
* ipa-cp.c (initialize_node_lattices): Use cgraph_local_p
to handle instrumentation clones properly.
(propagate_constants_accross_call): Do not propagate
through instrumentation thunks.
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 5e702a7..b225ebe 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -1556,4 +1556,17 @@ symtab_in_same_comdat_p (symtab_node *one, symtab_node
*two)
{
return DECL_COMDAT_GROUP (one->decl) == DECL_COMDAT_GROUP (two->decl);
}
+
+/* Return true if NODE is local. Instrumentation clones are counted as local
Re: [PATCH, Pointer Bounds Checker 28/x] IPA CP
2014-06-11 13:45 GMT+04:00 Martin Jambor :
> Hi,
>
> On Wed, Jun 11, 2014 at 12:24:57PM +0400, Ilya Enkovich wrote:
>> Hi,
>>
>> This patch fixes IPA CP pass to handle instrumented code correctly.
>>
>> Bootstrapped and tested on linux-x86_64.
>>
>> Thanks,
>> Ilya
>> --
>> gcc/
>>
>> 2014-06-11 Ilya Enkovich
>>
>> * ipa-cp.c (initialize_node_lattices): Check original
>> version locality for instrumentation clones.
>> (propagate_constants_accross_call): Do not propagate
>> through instrumentation thunks.
>>
>>
>> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
>> index 689378a..683b9f0 100644
>> --- a/gcc/ipa-cp.c
>> +++ b/gcc/ipa-cp.c
>> @@ -699,7 +699,10 @@ initialize_node_lattices (struct cgraph_node *node)
>>int i;
>>
>>gcc_checking_assert (cgraph_function_with_gimple_body_p (node));
>> - if (!node->local.local)
>> + if (!node->local.local
>> + || (node->instrumentation_clone
>> + && node->instrumented_version
>> + && !node->instrumented_version->local.local))
>
> This looks quite convoluted, can you please put the test into a new
> predicate in cgraph.c? I assume you had to change other tests of the
> local flag in a similar fashion anyway.
You are right. Would cgraph_node_local_p be OK?
>
>> {
>>/* When cloning is allowed, we can assume that externally visible
>>functions are not called. We will compensate this by cloning
>> @@ -1440,7 +1443,8 @@ propagate_constants_accross_call (struct cgraph_edge
>> *cs)
>>alias_or_thunk = cs->callee;
>>while (alias_or_thunk->alias)
>> alias_or_thunk = cgraph_alias_target (alias_or_thunk);
>> - if (alias_or_thunk->thunk.thunk_p)
>> + if (alias_or_thunk->thunk.thunk_p
>> + && !alias_or_thunk->thunk.add_pointer_bounds_args)
>
> so there are thunks that do not change the first argument and so we do
> want to propagate to/through it?
Yes. Thunks marked as add_pointer_bounds_args do not change arguments,
but add default pointer bounds for all pointer arguments.
>> {
>>ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
>> 0));
>> @@ -1449,6 +1453,20 @@ propagate_constants_accross_call (struct cgraph_edge
>> *cs)
>>else
>> i = 0;
>>
>> + /* No propagation through instrumentation thunks is available yet.
>> + It should be possible with proper mapping of call args and
>> + instrumented callee params in the propagation loop below. But
>> + this case mostly occurs when legacy code calls instrumented code
>> + and it is not a primary target for optimizations. */
>> + if (!alias_or_thunk->instrumentation_clone
>> + && callee->instrumentation_clone)
>> +{
>> + for (; i < parms_count; i++)
>> + ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
>> + i));
>> + return ret;
>> +}
>> +
>
> and these thunks are different from those marked as
> thunk.add_pointer_bounds_args? If they are not, the previous hunk is
> redundant.
This check covers more cases. It catches all chains of aliases and
thunks where thunk.add_pointer_bounds_args is met. It does not mean
the first met thunk has thunk.add_pointer_bounds_args (as is in
previous check). I suppose you are right about previous hunk. It
would be better to make wider check first and make exit earlier. Will
fix it.
>
> My apologies for not looking at the patches introducing all the new
> cgraph_node fields but it is quite difficult to figure out what the
> new tests actually mean (that is why I'd really prefer predicates with
> more explanatory names).
New predicates are good but do we need them for single usage? Locality
check are used during output and new predicate would be nice for it.
But there is no another thunk.add_pointer_bounds_args chain check used
somewhere else. Will try to make more explanatory comment for it for
now.
Thanks,
Ilya
>
> Thanks,
>
> Martin
Re: [PATCH, Pointer Bounds Checker 28/x] IPA CP
Hi,
On Wed, Jun 11, 2014 at 12:24:57PM +0400, Ilya Enkovich wrote:
> Hi,
>
> This patch fixes IPA CP pass to handle instrumented code correctly.
>
> Bootstrapped and tested on linux-x86_64.
>
> Thanks,
> Ilya
> --
> gcc/
>
> 2014-06-11 Ilya Enkovich
>
> * ipa-cp.c (initialize_node_lattices): Check original
> version locality for instrumentation clones.
> (propagate_constants_accross_call): Do not propagate
> through instrumentation thunks.
>
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 689378a..683b9f0 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -699,7 +699,10 @@ initialize_node_lattices (struct cgraph_node *node)
>int i;
>
>gcc_checking_assert (cgraph_function_with_gimple_body_p (node));
> - if (!node->local.local)
> + if (!node->local.local
> + || (node->instrumentation_clone
> + && node->instrumented_version
> + && !node->instrumented_version->local.local))
This looks quite convoluted, can you please put the test into a new
predicate in cgraph.c? I assume you had to change other tests of the
local flag in a similar fashion anyway.
> {
>/* When cloning is allowed, we can assume that externally visible
>functions are not called. We will compensate this by cloning
> @@ -1440,7 +1443,8 @@ propagate_constants_accross_call (struct cgraph_edge
> *cs)
>alias_or_thunk = cs->callee;
>while (alias_or_thunk->alias)
> alias_or_thunk = cgraph_alias_target (alias_or_thunk);
> - if (alias_or_thunk->thunk.thunk_p)
> + if (alias_or_thunk->thunk.thunk_p
> + && !alias_or_thunk->thunk.add_pointer_bounds_args)
so there are thunks that do not change the first argument and so we do
want to propagate to/through it?
> {
>ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
> 0));
> @@ -1449,6 +1453,20 @@ propagate_constants_accross_call (struct cgraph_edge
> *cs)
>else
> i = 0;
>
> + /* No propagation through instrumentation thunks is available yet.
> + It should be possible with proper mapping of call args and
> + instrumented callee params in the propagation loop below. But
> + this case mostly occurs when legacy code calls instrumented code
> + and it is not a primary target for optimizations. */
> + if (!alias_or_thunk->instrumentation_clone
> + && callee->instrumentation_clone)
> +{
> + for (; i < parms_count; i++)
> + ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
> + i));
> + return ret;
> +}
> +
and these thunks are different from those marked as
thunk.add_pointer_bounds_args? If they are not, the previous hunk is
redundant.
My apologies for not looking at the patches introducing all the new
cgraph_node fields but it is quite difficult to figure out what the
new tests actually mean (that is why I'd really prefer predicates with
more explanatory names).
Thanks,
Martin
