[google-4_6] indirect_call promotion for streaming LIPO (issue 6306054)

[Rong Xu]

On Thu, Jun 7, 2012 at 3:22 PM, <xur@google.com> wrote:
>
> will send the new patch set in a few minutes.
>
>
>
> http://codereview.appspot.com/6306054/diff/1/cgraph.h
> File cgraph.h (right):
>
> http://codereview.appspot.com/6306054/diff/1/cgraph.h#newcode410
> cgraph.h:410:
> They are referenced in lto-cgraph.c etc.
>
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> Why not putting this into value-prof.h?
>
>
> http://codereview.appspot.com/6306054/diff/1/ipa-split.c
> File ipa-split.c (right):
>
> http://codereview.appspot.com/6306054/diff/1/ipa-split.c#newcode1314
> ipa-split.c:1314: && (!flag_lto || flag_ripa_stream ||
> !node->local.externally_visible))
> will add. it just tries to sync the behavior with FE lipo. good for
> performance triage.
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> comment here?
>
>
> http://codereview.appspot.com/6306054/diff/1/lto-streamer-in.c
> File lto-streamer-in.c (right):
>
> http://codereview.appspot.com/6306054/diff/1/lto-streamer-in.c#newcode996
> lto-streamer-in.c:996: if (input_types_compatible_p (TREE_TYPE (tem),
> yes. this is a lto streaming issue.
>
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> Is this a general LTO fix?
>
>
> http://codereview.appspot.com/6306054/diff/1/lto-streamer-in.c#newcode1153
> lto-streamer-in.c:1153:
> Done.
>
> also fix the possible memory leak by using a local var for val.
>
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> More comments needed here.
>
>
> http://codereview.appspot.com/6306054/diff/1/value-prof.c
> File value-prof.c (right):
>
> http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1252
> value-prof.c:1252: /* When we replace cgraph_node with prevailing_node,
> update the
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> capital for parameter.
>
> Done
>
>
> http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1660
> value-prof.c:1660: gimple_ic_transform_mult_targ2 (gimple stmt,
> Done.
>
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> Since this is a helper function, change the name to
>
>
>> gimple_ic_transform_mult_targ_1
>
>
> http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1712
> value-prof.c:1712: if (val2 && ((count2 * 2) >= (all - count1))
> We will sync this two later. Don't want to change behavior for current
> lipo in this patch.
>
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> This heuristics need to be unified at some point.
>
>
> http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1717
> value-prof.c:1717: if (direct_call1 && !direct_call1->decl)
> this won't cause problem.
> this must be my code that works around some issues.
> I will remove and test it.
>
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> Why null decl? is it a bug?
>
>

It turns out that I cannot remove these stmt.
Without them results in ICE for some program. I'll keep it for now and
will investigate later.

> http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1727
> value-prof.c:1727: init_icall_promotion_info_htab();
> Fixed
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> space.
>
>
> http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1858
> value-prof.c:1858: icall_counters[4] = gimple_bb (stmt)->count;
> Will do this in later changes.
>
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> It is better to put the total count as separate parameter or at index
>
> 0 -- for
>>
>> future extension of more than 2 targets (The data is already there).
>
>
> http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1868
> value-prof.c:1868: gimple_ic_transform_mult_targ1 (struct cgraph_edge
> *call_edge)
> OK. Changed.
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> A better name -- maybe cgraph_ic_transform_mult_targ, since it takes a
>
> cgraph
>>
>> edge?
>
>
> Changed.
>
>
> http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1878
> value-prof.c:1878: val = (icall_promotion_info_t *) XNEW
> (icall_promotion_info_t);
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> XNEWC -- or use a local variable and initialize to 0.
>
>
> good catch. I'll use a local var.
>
>
> http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1881
> value-prof.c:1881: slot = htab_find_slot (icall_promotion_info_htab,
> val, NO_INSERT);
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> Why does it need to do look up?
>
>
> all the targets/counters info are in the hashtable.
>
> http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1884
> value-prof.c:1884:
>
> On 2012/06/07 21:46:53, davidxl wrote:
>>
>> Memory leak here -- or use local variable vor val.
>
> change to use a local var.
>
> http://codereview.appspot.com/6306054/