Re: [google gcc-4_7] new module grouping method (issue 7393058)

[davidxl at google dot com]

The coverage.c related patch is not uploaded properly. Will be reviewed
seperately.

David


https://codereview.appspot.com/7393058/diff/1/gcc/gcov-dump.c
File gcc/gcov-dump.c (right):

https://codereview.appspot.com/7393058/diff/1/gcc/gcov-dump.c#newcode581
gcc/gcov-dump.c:581: const char *primary_suffix = mod_info->is_primary ?
"primary" : "";
Put 'auxiilary' here

https://codereview.appspot.com/7393058/diff/1/gcc/gcov-dump.c#newcode583
gcc/gcov-dump.c:583: ? mod_info->is_primary ? "exported" : "auxiliary"
empty string here.

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
algo --> algorithm.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode79
libgcc/dyn-ipa.c:79: module ident.  */
module ident or module idx ?

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode92
libgcc/dyn-ipa.c:92: /* used by new_alg  */
new_algo --> new algorithm.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode98
libgcc/dyn-ipa.c:98: struct modu_node {
{ to next line.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode102
libgcc/dyn-ipa.c:102: unsigned char visited; /* needed? */
Remove the comment 'needed?'

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode113
libgcc/dyn-ipa.c:113: unsigned char visited; /* needed? */
Remove the field comment.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode189
libgcc/dyn-ipa.c:189: static int flag_modu_merge_edges = 1;
These two flags should have corresponding --param=

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode194
libgcc/dyn-ipa.c:194: #endif
Use __gcov_lipo_max_mem which has the value specified by
--param=max-lipo-mem=...

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode253
libgcc/dyn-ipa.c:253:
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#newcode292
libgcc/dyn-ipa.c:292:
Use either get_module_idx or get_module_id_value.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode298
libgcc/dyn-ipa.c:298: pointer_set_find_or_insert (p, get_module_id_value
(imp_mod));
Why not changing it to use get_module_idx?

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode333
libgcc/dyn-ipa.c:333: return get_module_id_value ((const struct
gcov_info *)p);
get_module_idx?

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode339
libgcc/dyn-ipa.c:339: gcc_assert (m_ix != 0);
module id may be insane. Should be handled here.

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;
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.

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;
space

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode344
libgcc/dyn-ipa.c:344: create_exported_to (unsigned m_ix)
See above comment about the ident.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode348
libgcc/dyn-ipa.c:348: gcc_assert (m_ix != 0);
See above comment about assertion.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode349
libgcc/dyn-ipa.c:349: the_dyn_call_graph.sup_modules[m_ix-1].exported_to
= p
space

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode355
libgcc/dyn-ipa.c:355: get_imported_modus (unsigned m_ix)
See above comment about parameter name.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode360
libgcc/dyn-ipa.c:360: gcc_assert (m_ix != 0);
Assertion needs to be handled.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode367
libgcc/dyn-ipa.c:367: = pointer_set_create (imp_mod_get_key);
split this into two statements

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode456
libgcc/dyn-ipa.c:456: if (flag_alg_mode == 1)
Define enum for algorithms.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode508
libgcc/dyn-ipa.c:508: pointer_set_destroy_2
(the_dyn_call_graph.sup_modules[i].exported_to);
Change the name for pointer_set_destroy_2.

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)
better interface name.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode731
libgcc/dyn-ipa.c:731: XDELETEVEC (pset->slots);
Unused i?

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.
The comment is not matching.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode828
libgcc/dyn-ipa.c:828: n = 0;
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);
wrap long line.

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)
wrap long line.

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)
wrap long line.

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
One new line above.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1542
libgcc/dyn-ipa.c:1542: case 0:
Name methods with _1, _0 properly: _1 is
'inclusion_based_with_priority'. The _0 is the old algorithm:
'eager_propagation'.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1550
libgcc/dyn-ipa.c:1550: {
make the parameter consistent -- e.g., consistently use ident.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1569
libgcc/dyn-ipa.c:1569: e = XNEW (struct modu_edge);
use XCNEW

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,
Missing comment.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1613
libgcc/dyn-ipa.c:1613: = XNEWVEC (struct modu_node, n_modules);
Use XCNEWVEC

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1630
libgcc/dyn-ipa.c:1630: gcc_assert (node);
Avoid assertion -- add error prints -- similarly for other assertions.

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)
Missing comments.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1668
libgcc/dyn-ipa.c:1668: static gcov_unsigned_t
Missing comments

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);
wrap long line.

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);
This will cause t_mid to be added to the exported_set for all the aux
modules in s_imported_modules -- is this needed? t_mid should be only
added to the the root module of s_imported_mods?

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1735
libgcc/dyn-ipa.c:1735: ps_check_ggc_mem (const void *value,
Comment needed.

https://codereview.appspot.com/7393058/diff/1/libgcc/dyn-ipa.c#newcode1817
libgcc/dyn-ipa.c:1817: return 0;
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: */
Remove this.

https://codereview.appspot.com/7393058/