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?
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/
