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/
