Re: [PATCH][google] Fix parallel make issue exposed by recent gcda change (issue 7426043)

Looks good to me. https://codereview.appspot.com/7426043/

Re: [PATCH][google] Fix parallel make issue exposed by recent gcda change (issue 7426043)

https://codereview.appspot.com/7426043/diff/1/gcc/gcov-io.h File gcc/gcov-io.h (right): https://codereview.appspot.com/7426043/diff/1/gcc/gcov-io.h#newcode1013 gcc/gcov-io.h:1013: GCOV_LINKAGE void gcov_seek (gcov_position_t /*position*/, int /* from_end */) I think it is better to create a new

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#newcode235 libgcc/dyn-ipa.c:235: /* Return module_id. FUNC_GUID is the global unique id. */ Add a comment here that the returned val

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

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 *primar

Re: [google 4.7] fdo build for linux kernel (issue 6968046)

The change in gcov-io.h is from a different patch. David https://codereview.appspot.com/6968046/diff/1/gcc/gcov-io.c File gcc/gcov-io.c (right): https://codereview.appspot.com/6968046/diff/1/gcc/gcov-io.c#newcode688 gcc/gcov-io.c:688: Have you compared this with this impl: while (x) { c++;

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

Ok for google branches. David http://codereview.appspot.com/6427063/

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h File gcc/gcov-io.h (right): http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode688 gcc/gcov-io.h:688: gcov_unsigned_t index; /* The corresponding string table index */ Is this field necessary? http://codereview.

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

On 2012/08/24 21:51:24, cmang wrote: Fixed formatting issues. The gcov.c is still not uploaded properly. === --- gcc/gcov.c (revision 190359) +++ gcc/gcov.c (working copy) @@ -222,6 +222,7 @@ typedef struct pmu_data { ll_

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

Where is the string table management code? The gcov.c file is not properly uploaded either. David http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c File gcc/gcov-io.c (right): http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode280 gcc/gcov-io.c:280: gcov_read_pmu

Re: [PATCH] New fdo summary-based icache sensitive unrolling (issue 6351086)

http://codereview.appspot.com/6351086/diff/1/gcc/gcov-io.h File gcc/gcov-io.h (right): http://codereview.appspot.com/6351086/diff/1/gcc/gcov-io.h#newcode396 gcc/gcov-io.h:396: gcov_unsigned_t num_hot_counters;/* number of counters to reach a given Should it be made into an array accessed with pr

Re: User directed Function Multiversioning via Function Overloading (issue 5752064)

http://codereview.appspot.com/5752064/diff/51001/gcc/cgraph.c File gcc/cgraph.c (right): http://codereview.appspot.com/5752064/diff/51001/gcc/cgraph.c#newcode1282 gcc/cgraph.c:1282: is needed as the address can be used to do an indirect call. */ Extend the comment here. http://codereview.appsp

Re: [google-4_6] indirect_call promotion for streaming LIPO (issue 6306054)

ok for google branches -- LTO specific bugs should be isolated and submitted upstream. David http://codereview.appspot.com/6306054/diff/5001/value-prof.c File value-prof.c (right): http://codereview.appspot.com/6306054/diff/5001/value-prof.c#newcode1720 value-prof.c:1720: direct_call2 = 0; Ok

Re: [google-4_6] indirect_call promotion for streaming LIPO (issue 6306054)

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: Why not putting this into value-prof.h? http://codereview.appspot.com/6306054/diff/1/ipa-split.c File ipa-split.c (right): http://coderevi

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

other than the update issue, good for google branch. David http://codereview.appspot.com/6282045/diff/4003/libgcc/libgcov.c File libgcc/libgcov.c (right): http://codereview.appspot.com/6282045/diff/4003/libgcc/libgcov.c#newcode1127 libgcc/libgcov.c:1127: cs_prg->num_hot_counters = cs_tprg->num

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h File gcc/gcov-io.h (right): http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode544 gcc/gcov-io.h:544: gcov_unsigned_t sum_cutoff_percent;/* sum_all cutoff percentage computed Is there a need to record this? http://coderev

Re: [google] libgcov workaround for weak reference issue (issue 6276043)

Ok with better naming for the dummy function such as '__gcov_dummy_ref1'. Another choice is to let __gcov_flush calls __gcov_dump + __gcov_reset -- but the dump_completed state needs to be saved and restored. David http://codereview.appspot.com/6276043/

Re: [PATCH] Take branch misprediction effects into account when RTL loop unrolling (issue 6099055)

It might be better to separate the data structure name change (niter_desc to loop_desc) into a different patch. Other than that, the patch looks good to me (for google branches only) Unroller really needs more heuristics like this instead of just looking at size. David http://codereview.appspot

Re: Backported r185231 from trunk. (issue 6139063)

Ok for google branches (please also backport to google/gcc_47 branch. David On 2012/05/01 20:37:44, asharif wrote: On 2012/04/30 19:54:14, asharif wrote: > I backported the following patch: > > 2012-03-12 Richard Guenther > >* gthr.h (__GTHREAD_MUTEX_INIT_FUNCT

Re: [PATCH] Take branch misprediction effects into account when RTL loop unrolling (issue 6099055)

http://codereview.appspot.com/6099055/diff/1/loop-unroll.c File loop-unroll.c (right): http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode156 loop-unroll.c:156: static bool An empty line here. http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode182 loop-unroll.c:182

Re: [Patch, i386] Avoid LCP stalls (issue 5975045)

http://codereview.appspot.com/5975045/diff/6001/config/i386/i386.md File config/i386/i386.md (right): http://codereview.appspot.com/5975045/diff/6001/config/i386/i386.md#newcode16974 config/i386/i386.md:16974: ;; gets too big. The comments may need to be updated. http://codereview.appspot.com/5

Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)

ok for google branches after checkin validation. David http://codereview.appspot.com/5851044/

Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)

It would be nice to add some unit/regression test cases of some sort. David http://codereview.appspot.com/5851044/diff/1/callgraph.c File callgraph.c (right): http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode309 callgraph.c:309: if (!is_prefix_of ("_ZL", name)) How about static

Re: [google][4.6]Make option -freorder-functions= invoke function reordering linker plugin (issue 5825054)

Ok for google branches after updating the doc/invoke.texi file. David http://codereview.appspot.com/5825054/

Re: LIPO based on LTO IR streaming (issue 5746044)

ok for google-46 after the minor changes below. Make sure default lipo testing is well covered too. David http://codereview.appspot.com/5746044/diff/11001/cgraph.c File cgraph.c (right): http://codereview.appspot.com/5746044/diff/11001/cgraph.c#newcode2887 cgraph.c:2887: && !(L_IPO_STREAM_IN_L

Re: Static branch prediction: compare IV to loop bound variable (issue 5504086)

Ok for google branches for now. thanks, David http://codereview.appspot.com/5504086/diff/1004/gcc/predict.c File gcc/predict.c (right): http://codereview.appspot.com/5504086/diff/1004/gcc/predict.c#newcode958 gcc/predict.c:958: find_qualified_ssa_name (tree t1, tree t2) Better change the name

Re: [4.7][google] Adding a new option "-fstack-protector-strong". (issue 5461043)

ok for google branches with the above changes. Please continue to seek upstream approval. David http://codereview.appspot.com/5461043/diff/19001/gcc/doc/invoke.texi File gcc/doc/invoke.texi (right): http://codereview.appspot.com/5461043/diff/19001/gcc/doc/invoke.texi#newcode403 gcc/doc/invoke.

Re: [4.7][google] Adding a new option "-fstack-protector-strong". (issue 5461043)

Also need to update doc/invoke.texi file for the new option. 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#newcode1531 gcc/cfgexpand.c:1531: record_or_union_type_has_array (const

Re: [google][4.6]Add new target builtin to check for amdfam15h processors (issue 5535046)

http://codereview.appspot.com/5535046/diff/1/gcc/config/i386/i386.c File gcc/config/i386/i386.c (right): http://codereview.appspot.com/5535046/diff/1/gcc/config/i386/i386.c#newcode26032 gcc/config/i386/i386.c:26032: +M_AMDFAM15, Maybe better to change 10 to 10H, and 15 to 15H in the name as

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

Ok, Cary's explanation makes sense. Please update the comments to make it clearer. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c File gcc/config/i386/i386.c (right): http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927 gcc/config/i386/i386.c:109

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

http://codereview.appspot.com/5416043/diff/12001/gcc/config/i386/i386.c File gcc/config/i386/i386.c (left): http://codereview.appspot.com/5416043/diff/12001/gcc/config/i386/i386.c#oldcode10928 gcc/config/i386/i386.c:10928: if (current_function_decl != NULL_TREE && I am not sure how the hack you

Re: [google][4.6]Compiler Directed Multiversioning with new -mvarch option (issue 5490054)

http://codereview.appspot.com/5490054/diff/1011/config/i386/i386.c File config/i386/i386.c (right): http://codereview.appspot.com/5490054/diff/1011/config/i386/i386.c#newcode26569 config/i386/i386.c:26569: +mversion_for_core2 (tree *optimization_node, -> mversionable_for_core2_p ? http://codere

Re: [PATCH i386][google]With -mtune=core2, avoid generating the slow unaligned vector load/store (issue 5488054)

http://codereview.appspot.com/5488054/diff/4002/tree-vect-stmts.c File tree-vect-stmts.c (right): http://codereview.appspot.com/5488054/diff/4002/tree-vect-stmts.c#newcode3712 tree-vect-stmts.c:3712: } The check can be put into a helper function. http://codereview.appspot.com/5488054/

Re: Fix compiler warnings in ThreadSanitizer tests (issue 5483046)

ok for google/main. David http://codereview.appspot.com/5483046/

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

http://codereview.appspot.com/5416043/diff/6001/gcc/config/i386/i386.c File gcc/config/i386/i386.c (right): http://codereview.appspot.com/5416043/diff/6001/gcc/config/i386/i386.c#newcode10881 gcc/config/i386/i386.c:10881: + '_function_patch_epilogue'. The backpointer section can be used to nav

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

;' by ix86_elf_asm_named_section(). */ On 2011/12/02 01:57:17, harshit wrote: On 2011/11/28 22:16:27, davidxl wrote: > Explain more on the comdat handling. I have limited knowledge about comdat sections, so can't give a detailed explanation on why the assembler emits an erro

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

Please also explain the need for backpointer section. David http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c File gcc/config/i386/i386.c (right): http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10801 gcc/config/i386/i386.c:10801: +static bool Add a

Re: [google] fix error caught by TREE_CHECKING (issue 5401045)

Ok for google branches. David http://codereview.appspot.com/5401045/diff/1/gcc/dwarf2out.c File gcc/dwarf2out.c (right): http://codereview.appspot.com/5401045/diff/1/gcc/dwarf2out.c#newcode19777 gcc/dwarf2out.c:19777: { Use TYPE_P (decl_context) http://codereview.appspot.com/5401045/

Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)

Have you run through SPEC, and SPEC06 with this change? What is the instrumentation overhead using gcc? David http://codereview.appspot.com/5303083/

Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)

http://codereview.appspot.com/5303083/diff/28001/gcc/tree-tsan.c File gcc/tree-tsan.c (right): http://codereview.appspot.com/5303083/diff/28001/gcc/tree-tsan.c#newcode227 gcc/tree-tsan.c:227: var = varpool_node_for_asm (id); Use cgraph_node_for_asm instead. http://codereview.appspot.com/5303083

Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)

http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c File gcc/tree-tsan.c (right): http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode432 gcc/tree-tsan.c:432: /* Builds either (__tsan_shadow_stack += 1) or (__tsan_shadow_stack -= 1) expression Line too long. http://code

Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)

http://codereview.appspot.com/5303083/diff/3002/gcc/tree-tsan.c File gcc/tree-tsan.c (right): http://codereview.appspot.com/5303083/diff/3002/gcc/tree-tsan.c#newcode1075 gcc/tree-tsan.c:1075: for (eidx = 0; VEC_iterate (edge, exit_bb->preds, eidx, e); eidx++) Use FOR_EACH_EDGE macro http://code

Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)

Have not done with reviewing. This is the first batch. David http://codereview.appspot.com/5303083/diff/1/gcc/passes.c File gcc/passes.c (right): http://codereview.appspot.com/5303083/diff/1/gcc/passes.c#newcode1423 gcc/passes.c:1423: NEXT_PASS (pass_tsan); Move this to the same place as asan.

Re: [google] AddressSanitizer for gcc, first attempt. (issue 5272048)

http://codereview.appspot.com/5272048/diff/18001/tree-asan.c File tree-asan.c (right): http://codereview.appspot.com/5272048/diff/18001/tree-asan.c#newcode325 tree-asan.c:325: base = build_addr (t, current_function_decl); There are issues with creating address expressions from TARGET_MEM_REF in

Re: [google] AddressSanitizer for gcc, first attempt. (issue 5272048)

http://codereview.appspot.com/5272048/diff/18001/tree-asan.c File tree-asan.c (right): http://codereview.appspot.com/5272048/diff/18001/tree-asan.c#newcode325 tree-asan.c:325: base = build_addr (t, current_function_decl); You need to create a temp var and build as gimple assignment. See init_tmp

Re: [google] AddressSanitizer for gcc, first attempt. (issue 5272048)

for asan when there is array ref with runtime index -- this is mainly for alias analysis. On 2011/10/17 23:04:50, kcc wrote: On 2011/10/17 22:33:17, davidxl wrote: > Discard 2) -- it is not correct. What Asan needs is slightly different. Yea. I actually have a test where this inst

Re: [google] AddressSanitizer for gcc, first attempt. (issue 5272048)

slightly different. David On 2011/10/17 22:26:55, davidxl wrote: Two suggestions: 1) You only need to deal with GIMPLE_ASSIGN (lhs and rhs) for all memory references) 2) use get_base_address function to compute base address. http://codereview.appspot.com/5272048/

Re: [google] AddressSanitizer for gcc, first attempt. (issue 5272048)

fasan option also needs to be documented in doc/invoke.texi. http://codereview.appspot.com/5272048/diff/2001/tree-asan.c File tree-asan.c (right): http://codereview.appspot.com/5272048/diff/2001/tree-asan.c#newcode54 tree-asan.c:54: ShadowValue = (char*)ShadowAddr; *(char*) ShadowAddr; http://

Re: [google] New linker plugin to do function reordering in the final binary using callgraph profiles (issue4802070)

Ok for google branches It is better to have a higher level compiler option to be introduced for this purpose instead of asking user to specify the plugin path. The option should enable 1) ffunction-sections; 2) cgraph note section genration; 3) enable the plugin. One possibility is to enhance th

[google] fix global vars incorrectly marked as read-only in LIPO mode (issue4798045)

http://codereview.appspot.com/4798045/diff/1/ipa.c File ipa.c (right): http://codereview.appspot.com/4798045/diff/1/ipa.c#newcode1034 ipa.c:1034: { Has varpool node linking happened at this point? If not, the new code here is not excersised. http://codereview.appspot.com/4798045/diff/1/ipa.c#ne

Re: [google] limit excessive load/store motions (issue4563044)

Ok for google/main after the minor cleanups. Incorporate comments from maintainers when available. David http://codereview.appspot.com/4563044/diff/1/gcc/gcse.c File gcc/gcse.c (right): http://codereview.appspot.com/4563044/diff/1/gcc/gcse.c#newcode5050 gcc/gcse.c:5050: if (ld_motion_count >=

Re: [google] Avoid reading struct member from structure generated by different gcc version (issue4446047)

LGTM http://codereview.appspot.com/4446047/diff/1/gcc/libgcov.c File gcc/libgcov.c (right): http://codereview.appspot.com/4446047/diff/1/gcc/libgcov.c#newcode155 gcc/libgcov.c:155: filename? filename : "", e, v); Better split it into two different printfs as the format is different -- otherwise