On Thu, Jun 7, 2012 at 3:22 PM, <x...@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/