On 11 Jun 15:07, Ilya Enkovich wrote:
> 2014-06-11 13:45 GMT+04:00 Martin Jambor <mjam...@suse.cz>:
> > 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 <ilya.enkov...@intel.com>
> >>
> >> * 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 <ilya.enkov...@intel.com>
* 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. */
