Re: [google gcc-4_7] offline profile merge tool (issue 8508048)
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py File contrib/profile_merge.py (right): https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode53 contrib/profile_merge.py:53: data_file = open(path, 'rb') On 2013/04/09 17:32:11, martint wrote: How about simpler version: with open(file, 'rb') as f: data = f.read() Done. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode59 contrib/profile_merge.py:59: def MergeCounters(objs, index, multipliers): changed the name to ReturnMergedCounters On 2013/04/09 17:32:11, martint wrote: Perhaps use function name that shows that it returns the value. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode78 contrib/profile_merge.py:78: length: Length of the data on the disk All the bullets sub-to Attributes are not capital. On 2013/04/09 17:32:11, martint wrote: Minor style. Make sure format is coherent (ends with . or not, starts with capital letter or not). https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode119 contrib/profile_merge.py:119: self.ident = 0 Probably not. 'reader' here is ProfileDataFile. We should have a valid gcda/gcno file for this program. But it does not hurt to have a empty reader. I'll leave as it's. On 2013/04/09 17:32:11, martint wrote: It is ever useful to have a function object with no reader? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode138 contrib/profile_merge.py:138: return self.ArcCounters().counters[0] that will not happen. all functions have at least one counter. On 2013/04/09 17:32:11, martint wrote: This will throw an exception if no counters exists. Is that expected? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode209 contrib/profile_merge.py:209: class Lines(DataObject): It's for reading in gcno file. We are not using this functionality for merge. On 2013/04/09 17:32:11, martint wrote: Where is this used? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1153 contrib/profile_merge.py:1153: os.chdir(direc) On 2013/04/09 17:32:11, martint wrote: How about using absolute paths to avoid the chdir logic? Done. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1199 contrib/profile_merge.py:1199: outfile_path = output_path + '/' + f On 2013/04/09 17:32:11, martint wrote: os.path.join() instead of +, here and other places where the pattern is used. Done. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1213 contrib/profile_merge.py:1213: def Merge(self, archives, multipliers): The caller in this program always has self in archives set. On 2013/04/09 17:32:11, martint wrote: The code seems to handle both the case when 'self' is inside archives and when it is not. Does both happen when running the code? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1233 contrib/profile_merge.py:1233: for i, a in enumerate(archives): i is the i-th profile archive. On 2013/04/09 17:32:11, martint wrote: It's hard to see what i is in this case. I would prefer to have information about arguments in the documentation at the top of the function, but more descriptive variable names and comments would also help. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1313 contrib/profile_merge.py:1313: input_profiles[0].Merge(input_profiles, profile_multipliers) That will use more memory. We already use too much memory. And it only modifies in-memory object (the profile files are not changed). On 2013/04/09 17:32:11, martint wrote: Did you conside creating a new object instead of modifying the existing? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1317 contrib/profile_merge.py:1317: input_profiles[0].Write(output_profile) Refer to previous comment. Only in-memory objects are overwritten. On 2013/04/09 17:32:11, martint wrote: Warn before overwriting? https://codereview.appspot.com/8508048/
Re: [google gcc-4_7] new module grouping method (issue 7393058)
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)
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: Backported r185231 from trunk. (issue 6878045)
Look good to me. Please check with David if google-main branch is currently opened for check-in. -Rong https://codereview.appspot.com/6878045/
Re: [google-4_6] indirect_call promotion for streaming LIPO (issue 6306054)
will send the new patch set in a few minutes. http://codereview.appspot.com/6306054/diff/1/cgraph.h File cgraph.h (right): http://codereview.appspot.com/6306054/diff/1/cgraph.h#newcode410 cgraph.h:410: They are referenced in lto-cgraph.c etc. On 2012/06/07 21:46:53, davidxl wrote: Why not putting this into value-prof.h? http://codereview.appspot.com/6306054/diff/1/ipa-split.c File ipa-split.c (right): http://codereview.appspot.com/6306054/diff/1/ipa-split.c#newcode1314 ipa-split.c:1314: (!flag_lto || flag_ripa_stream || !node-local.externally_visible)) will add. it just tries to sync the behavior with FE lipo. good for performance triage. On 2012/06/07 21:46:53, davidxl wrote: comment here? http://codereview.appspot.com/6306054/diff/1/lto-streamer-in.c File lto-streamer-in.c (right): http://codereview.appspot.com/6306054/diff/1/lto-streamer-in.c#newcode996 lto-streamer-in.c:996: if (input_types_compatible_p (TREE_TYPE (tem), yes. this is a lto streaming issue. On 2012/06/07 21:46:53, davidxl wrote: Is this a general LTO fix? http://codereview.appspot.com/6306054/diff/1/lto-streamer-in.c#newcode1153 lto-streamer-in.c:1153: Done. also fix the possible memory leak by using a local var for val. On 2012/06/07 21:46:53, davidxl wrote: More comments needed here. http://codereview.appspot.com/6306054/diff/1/value-prof.c File value-prof.c (right): http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1252 value-prof.c:1252: /* When we replace cgraph_node with prevailing_node, update the On 2012/06/07 21:46:53, davidxl wrote: capital for parameter. Done http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1660 value-prof.c:1660: gimple_ic_transform_mult_targ2 (gimple stmt, Done. On 2012/06/07 21:46:53, davidxl wrote: Since this is a helper function, change the name to gimple_ic_transform_mult_targ_1 http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1712 value-prof.c:1712: if (val2 ((count2 * 2) = (all - count1)) We will sync this two later. Don't want to change behavior for current lipo in this patch. On 2012/06/07 21:46:53, davidxl wrote: This heuristics need to be unified at some point. http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1717 value-prof.c:1717: if (direct_call1 !direct_call1-decl) this won't cause problem. this must be my code that works around some issues. I will remove and test it. On 2012/06/07 21:46:53, davidxl wrote: Why null decl? is it a bug? http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1727 value-prof.c:1727: init_icall_promotion_info_htab(); Fixed On 2012/06/07 21:46:53, davidxl wrote: space. http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1858 value-prof.c:1858: icall_counters[4] = gimple_bb (stmt)-count; Will do this in later changes. On 2012/06/07 21:46:53, davidxl wrote: It is better to put the total count as separate parameter or at index 0 -- for future extension of more than 2 targets (The data is already there). http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1868 value-prof.c:1868: gimple_ic_transform_mult_targ1 (struct cgraph_edge *call_edge) OK. Changed. On 2012/06/07 21:46:53, davidxl wrote: A better name -- maybe cgraph_ic_transform_mult_targ, since it takes a cgraph edge? Changed. http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1878 value-prof.c:1878: val = (icall_promotion_info_t *) XNEW (icall_promotion_info_t); On 2012/06/07 21:46:53, davidxl wrote: XNEWC -- or use a local variable and initialize to 0. good catch. I'll use a local var. http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1881 value-prof.c:1881: slot = htab_find_slot (icall_promotion_info_htab, val, NO_INSERT); On 2012/06/07 21:46:53, davidxl wrote: Why does it need to do look up? all the targets/counters info are in the hashtable. http://codereview.appspot.com/6306054/diff/1/value-prof.c#newcode1884 value-prof.c:1884: On 2012/06/07 21:46:53, davidxl wrote: Memory leak here -- or use local variable vor val. change to use a local var. http://codereview.appspot.com/6306054/
Re: [4.7][google] Adding a new option -fstack-protector-strong. (issue 5461043)
OK for google branches. http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c File gcc/cfgexpand.c (right): http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1597 gcc/cfgexpand.c:1597: contain an array or are arrays. */ , before or, and two spaces before */. http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1709 gcc/cfgexpand.c:1709: create_stack_guard (); it's better to merge to the earlier if statement. something like: if (flag_stack_protect == 2 || (flag_stack_protect == 3 gen_stack_protect_signal) || (flag_stack_protect (cfun-calls_alloca || has_protected_decls))) create_stack_guard (); http://codereview.appspot.com/5461043/
