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

[xur]

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 

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

[Xinliang David Li]

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?


 

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

[davidxl]

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c
File libgcc/dyn-ipa.c (right):

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode235
libgcc/dyn-ipa.c:235: /* Return module_id. FUNC_GUID is the global
unique id.  */
Add a comment here that the returned value is 1 based.

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode238
libgcc/dyn-ipa.c:238: get_module_id_from_func_glob_uid (gcov_type
func_guid)
get_module_ident_ ... is better.

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode246
libgcc/dyn-ipa.c:246: get_module_id_value (const struct gcov_info
*module_info)
get_module_ident is better.

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode1212
libgcc/dyn-ipa.c:1212: mod_id = get_module_id_from_func_glob_uid
(node-guid) - 1;
Change var name from mod_id to mod_idx

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode1232
libgcc/dyn-ipa.c:1232: callee_mod_id
-- callee_mod_idx

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode1250
libgcc/dyn-ipa.c:1250: = get_module_info (callee_mod_id);
Should be get_module_info (callee_mod_idx + 1).

You should carefully check to make sure NO interfaces takes idx as
parameters, and no interface returns idx.

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode2234
libgcc/dyn-ipa.c:2234: /* Dumper function for NODE.   M is the module id
and F is the function id.  */
M is module ident - 1

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode2276
libgcc/dyn-ipa.c:2276: /* Dumper function for NODE.   M is the module id
and F is the function id.  */
M is module_ident -1

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

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

[xur]

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c
File libgcc/dyn-ipa.c (right):

https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode1212
libgcc/dyn-ipa.c:1212: mod_id = get_module_id_from_func_glob_uid
(node-guid) - 1;
Send the wrong patch.
Actually. This should be
mod_id = get_module_id_from_func_glob_uid (node-guid);

On 2013/02/27 00:40:31, davidxl wrote:

Change var name from mod_id to mod_idx


https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode1232
libgcc/dyn-ipa.c:1232: callee_mod_id
Same as above. -1 should be removed.

On 2013/02/27 00:40:31, davidxl wrote:

-- callee_mod_idx


https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode1250
libgcc/dyn-ipa.c:1250: = get_module_info (callee_mod_id);
This is ok.

On 2013/02/27 00:40:31, davidxl wrote:

Should be get_module_info (callee_mod_idx + 1).



You should carefully check to make sure NO interfaces takes idx as

parameters,

and no interface returns idx.


https://codereview.appspot.com/7393058/diff/8001/libgcc/dyn-ipa.c#newcode2276
libgcc/dyn-ipa.c:2276: /* Dumper function for NODE.   M is the module id
and F is the function id.  */
On 2013/02/27 00:40:31, davidxl wrote:

M is module_ident -1


Done.

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

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

[davidxl]

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