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
