Can you upload the new patch set? David
On Tue, Feb 26, 2013 at 1:50 PM, <x...@google.com> wrote: > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c > File libgcc/dyn-ipa.c (right): > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode77 > libgcc/dyn-ipa.c:77: /* Used by new algo. This dyn_pointer_set only > On 2013/02/26 00:49:17, davidxl wrote: >> >> algo --> algorithm. > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode79 > libgcc/dyn-ipa.c:79: module ident. */ > On 2013/02/26 00:49:17, davidxl wrote: >> >> module ident or module idx ? > > > it's module ident. ie we should not see 0 as the key. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode92 > libgcc/dyn-ipa.c:92: /* used by new_alg */ > On 2013/02/26 00:49:17, davidxl wrote: >> >> new_algo --> new algorithm. > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode98 > libgcc/dyn-ipa.c:98: struct modu_node { > On 2013/02/26 00:49:17, davidxl wrote: >> >> { to next line. > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode102 > libgcc/dyn-ipa.c:102: unsigned char visited; /* needed? */ > On 2013/02/26 00:49:17, davidxl wrote: >> >> Remove the comment 'needed?' > > > removed this field as it's not used. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode113 > libgcc/dyn-ipa.c:113: unsigned char visited; /* needed? */ > On 2013/02/26 00:49:17, davidxl wrote: >> >> Remove the field comment. > > done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode189 > libgcc/dyn-ipa.c:189: static int flag_modu_merge_edges = 1; > On 2013/02/26 00:49:17, davidxl wrote: >> >> These two flags should have corresponding --param= > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode194 > libgcc/dyn-ipa.c:194: #endif > On 2013/02/26 00:49:17, davidxl wrote: >> >> Use __gcov_lipo_max_mem which has the value specified by >> --param=max-lipo-mem=... > > > Done. > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode253 > libgcc/dyn-ipa.c:253: > The ident is 1 based (started from 1) while idx is 0 based. > In the original implementation, ident is used as pointer_set key, and > idx is used to access the static arrays. Here I'm using the same way. > But it's pretty confusing. I like the idea of using ident exclusively. > we will be wasting a little bit memory but the code is cleaner. > > I'll change this in my up-coming patch. > > > On 2013/02/26 00:49:17, davidxl wrote: >> >> Why is this interface needed? Can get_module_idx be used instead? Or > > get rid of >> >> get_module_idx, and use ident consistently (and fix up limited array > > reference >> >> by 1). get_module_idx_from_guid should also be changed to >> get_module_ident_from_guid > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode339 > libgcc/dyn-ipa.c:339: gcc_assert (m_ix != 0); > On 2013/02/26 00:49:17, davidxl wrote: >> >> module id may be insane. Should be handled here. > > > I was thinking to add a check here. But we don't found a check in > get_module_idx() or inserting the ident in imp_mod_set_insert. So I > thought the insane id is not that often. > > I'll add a check here. I need to think what to do if we see an insane > id. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode340 > libgcc/dyn-ipa.c:340: return > the_dyn_call_graph.sup_modules[m_ix-1].exported_to; > On 2013/02/26 00:49:17, davidxl wrote: >> >> The input m_ix should be normalized to zero based idx already. If >> get_module_ident is used consistently, the parameter name should be > > changed to >> >> module_ident. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode367 > libgcc/dyn-ipa.c:367: = pointer_set_create (imp_mod_get_key); > On 2013/02/26 00:49:17, davidxl wrote: >> >> split this into two statements > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode456 > libgcc/dyn-ipa.c:456: if (flag_alg_mode == 1) > On 2013/02/26 00:49:17, davidxl wrote: >> >> Define enum for algorithms. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode728 > libgcc/dyn-ipa.c:728: pointer_set_destroy_2 (struct dyn_pointer_set > *pset) > On 2013/02/26 00:49:17, davidxl wrote: >> >> better interface name. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode731 > libgcc/dyn-ipa.c:731: XDELETEVEC (pset->slots); > On 2013/02/26 00:49:17, davidxl wrote: >> >> Unused i? > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode810 > libgcc/dyn-ipa.c:810: /* Returns nonzero if PSET contains P. P must be > nonnull. > Fixed > > On 2013/02/26 00:49:17, davidxl wrote: >> >> The comment is not matching. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode828 > libgcc/dyn-ipa.c:828: n = 0; > Not likely. all the slots (upon creation and expansion) are initialized > to 0. > > > On 2013/02/26 00:49:17, davidxl wrote: >> >> Is it possible that there is no empty slot -- thus this cause infinite > > loop? > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1254 > libgcc/dyn-ipa.c:1254: static int dyn_fibheap_comp_data (dyn_fibheap_t, > dyn_fibheapkey_t, void *, fibnode_t); > On 2013/02/26 00:49:17, davidxl wrote: >> >> wrap long line. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1281 > libgcc/dyn-ipa.c:1281: dyn_fibheap_compare (dyn_fibheap_t heap > ATTRIBUTE_UNUSED, fibnode_t a, fibnode_t b) > On 2013/02/26 00:49:17, davidxl wrote: >> >> wrap long line. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1291 > libgcc/dyn-ipa.c:1291: dyn_fibheap_comp_data (dyn_fibheap_t heap, > dyn_fibheapkey_t key, void *data, fibnode_t b) > On 2013/02/26 00:49:17, davidxl wrote: >> >> wrap long line. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1532 > libgcc/dyn-ipa.c:1532: /* Compute module grouping using CUTOFF_COUNT as > the hot edge > On 2013/02/26 00:49:17, davidxl wrote: >> >> One new line above. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1542 > libgcc/dyn-ipa.c:1542: case 0: > On 2013/02/26 00:49:17, davidxl wrote: >> >> Name methods with _1, _0 properly: _1 is > > 'inclusion_based_with_priority'. The _0 >> >> is the old algorithm: 'eager_propagation'. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1569 > libgcc/dyn-ipa.c:1569: e = XNEW (struct modu_edge); > On 2013/02/26 00:49:17, davidxl wrote: >> >> use XCNEW > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1582 > libgcc/dyn-ipa.c:1582: modu_graph_process_dyn_cgraph_node (struct > dyn_cgraph_node *node, > On 2013/02/26 00:49:17, davidxl wrote: >> >> Missing comment. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1613 > libgcc/dyn-ipa.c:1613: = XNEWVEC (struct modu_node, n_modules); > On 2013/02/26 00:49:17, davidxl wrote: >> >> Use XCNEWVEC > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1630 > libgcc/dyn-ipa.c:1630: gcc_assert (node); > On 2013/02/26 00:49:17, davidxl wrote: >> >> Avoid assertion -- add error prints -- similarly for other assertions. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1660 > libgcc/dyn-ipa.c:1660: get_group_ggc_mem (struct dyn_pointer_set *s) > On 2013/02/26 00:49:17, davidxl wrote: >> >> Missing comments. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1668 > libgcc/dyn-ipa.c:1668: static gcov_unsigned_t > On 2013/02/26 00:49:17, davidxl wrote: >> >> Missing comments > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1727 > libgcc/dyn-ipa.c:1727: pointer_set_traverse (s_imported_mods, > modu_add_auxiliary_1, &t_mid, &count, 0); > On 2013/02/26 00:49:17, davidxl wrote: >> >> wrap long line. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1727 > libgcc/dyn-ipa.c:1727: pointer_set_traverse (s_imported_mods, > modu_add_auxiliary_1, &t_mid, &count, 0); > On 2013/02/26 00:49:17, davidxl wrote: >> >> wrap long line. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1735 > libgcc/dyn-ipa.c:1735: ps_check_ggc_mem (const void *value, > On 2013/02/26 00:49:17, davidxl wrote: >> >> Comment needed. > > > Done. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1817 > libgcc/dyn-ipa.c:1817: return 0; > Will discuss this off-line. > > > On 2013/02/26 00:49:17, davidxl wrote: >> >> This constraints might be too strong. For instance, if one of the > > exported_to >> >> module (which is not the hottest) has the memory limit exceeded -- it > > will block >> >> other exported_to modules from benefiting from the newly added aux > > module. > >> Since the the edges are processed in priority order, it should ok to > > relax this >> >> to allow propagation even when exported_to module is too large. This > > still has >> >> the good nature of the inclusion based algo -- because the most > > critical part of >> >> the aux groups are still shared in the hierarchy. > > > https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode2116 > libgcc/dyn-ipa.c:2116: */ > On 2013/02/26 00:49:17, davidxl wrote: >> >> Remove this. > > > Done. > > https://codereview.appspot.com/7393058/
