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

Tue Feb 26 23:26:00 GMT 2013

Can you upload the new patch set?

David

On Tue, Feb 26, 2013 at 1:50 PM,  <xur@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/