Re: [PATCH] Convert more passes to new dump framework
On Fri, Aug 30, 2013 at 9:58 PM, Teresa Johnson tejohn...@google.com wrote: Besides, we might also want to use the same machinery (dump_printf_loc etc) for dump file dumping. The current behavior of using '-details' to turn on opt-info-all messages for dump files are not desirable. Interestingly, this doesn't even work. When I do -fdump-ipa-inline-details=stderr (with my patch containing the inliner messages) I am not getting those inliner messages emitted to stderr. Even though in dumpfile.c details is set to (TDF_DETAILS | MSG_OPTIMIZED_LOCATIONS | MSG_MISSED_OPTIMIZATION | MSG_NOTE). I'm not sure why, but will need to debug this. It works for vectorizer pass. Ok, let me see what is going on - I just confirmed that it is not working for the loop unroller messages either. Found the issue. The stream was incorrectly being closed when it was stderr/stdout. So only the dump output before the first dump_finish call was being emitted to stderr. I fixed this the same way the alt_dump_file was being handled just below - don't close if it is stderr/stdout. Confirmed that this fixes the problem. (So the real ratio between the volume of -fdump-...=stderr and -fopt-info is much higher than what I reported in an earlier email) Is the following patch ok, pending regression tests? 2013-08-30 Teresa Johnson tejohn...@google.com * dumpfile.c (dump_finish): Don't close stderr/stdout. Index: dumpfile.c === --- dumpfile.c (revision 202059) +++ dumpfile.c (working copy) @@ -450,7 +450,8 @@ dump_finish (int phase) if (phase 0) return; dfi = get_dump_file_info (phase); - if (dfi-pstream) + if (dfi-pstream strcmp(stderr, dfi-pfilename) != 0 + strcmp(stdout, dfi-pfilename) != 0) fclose (dfi-pstream); if (dfi-alt_stream strcmp(stderr, dfi-alt_filename) != 0 Yes, this is clearly a bug which I missed. Thanks for fixing it. Is it feasible to add a test case for it? Thanks, Sharad Thanks, Teresa
Re: [PATCH] Convert more passes to new dump framework
On 30 August 2013 23:23:16 Teresa Johnson tejohn...@google.com wrote: On Fri, Aug 30, 2013 at 1:30 PM, Xinliang David Li davi...@google.com wrote: On Fri, Aug 30, 2013 at 12:51 PM, Teresa Johnson tejohn...@google.com wrote: On Fri, Aug 30, 2013 at 9:27 AM, Xinliang David Li davi...@google.com wrote: Except that in this form, the dump will be extremely large and not suitable for very large applications. Yes. I did some measurements for both a fairly large source file that is heavily optimized with LIPO and for a simple toy example that has some inlining. For the large source file, the output from -fdump-ipa-inline=stderr was almost 100x the line count of the -fopt-info output. For the toy source file it was 43x. The size of the -details output was 250x and 100x, respectively. Which is untenable for a large app. The issue I am having here is that I want a more verbose message, not a more voluminous set of messages. Using either -fopt-info-all or -fdump-ipa-inline to provoke the more verbose inline message will give me a much greater volume of output. One compromise could be to emit the more verbose inliner message under a param (and a more concise foo inlined into bar by default with -fopt-info). Or we could do some variant of what David talks about below. something like --param=verbose-opt-info=1 Yes. Richard, would this be acceptable for now? i.e. the inliner messages would be like: -fopt-info: test.c:8:3: note: foobar inlined into foo with call count 9000 (the with call count X only when there is profile feedback) -fopt-info --param=verbose-opt-info=1: test.c:8:3: note: foobar/0 (9000) inlined into foo/2 (1000) with call count 9000 (via inline instance bar [3] (9000)) (again the call counts only emitted under profile feedback) Assuming the [3] is order, please change that to match what the in liner uses, I.e. /3 Thanks Besides, we might also want to use the same machinery (dump_printf_loc etc) for dump file dumping. The current behavior of using '-details' to turn on opt-info-all messages for dump files are not desirable. Interestingly, this doesn't even work. When I do -fdump-ipa-inline-details=stderr (with my patch containing the inliner messages) I am not getting those inliner messages emitted to stderr. Even though in dumpfile.c details is set to (TDF_DETAILS | MSG_OPTIMIZED_LOCATIONS | MSG_MISSED_OPTIMIZATION | MSG_NOTE). I'm not sure why, but will need to debug this. It works for vectorizer pass. Ok, let me see what is going on - I just confirmed that it is not working for the loop unroller messages either. How about the following: 1) add a new dump_kind modifier so that when that modifier is specified, the messages won't goto the alt_dumpfile (controlled by -fopt-info), but only to primary dump file. With this, the inline messages can be dumped via: dump_printf_loc (OPT_OPTIMIZED_LOCATIONS | OPT_DUMP_FILE_ONLY, .) (you mean (MSG_OPTIMIZED_LOCATIONS | OPT_DUMP_FILE_ONLY) ) Yes. Typically OR-ing together flags like this indicates dump under any of those conditions. But we could implement special handling for OPT_DUMP_FILE_ONLY, which in the above case would mean dump only to the primary dump file, and only under the other conditions specified in the flag (here under -optimized) 2) add more flags in -fdump- support: -fdump-ipa-inline-opt -- turn on opt-info messages only -fdump-ipa-inline-optall -- turn on opt-info-all messages According to the documentation (see the -fdump-tree- documentation on http://gcc.gnu.org/onlinedocs/gcc/Debugging-Options.html#Debugging-Options), the above are already supposed to be there (-optimized, -missed, -note and -optall). However, specifying any of these gives a warning like: cc1: warning: ignoring unknown option ‘optimized’ in ‘-fdump-ipa-inline’ [enabled by default] Probably because none is listed in the dump_options[] array in dumpfile.c. However, I don't think there is currently a way to use -fdump- options and *only* get one of these, as much of the current dump output is emitted whenever there is a dump_file defined. Until everything is migrated to the new framework it may be difficult to get this to work. -fdump-tree-pre-ir -- turn on GIMPLE dump only -fdump-tree-pre-details -- turn on everything (ir, optall, trace) With this, developers can really just use -fdump-ipa-inline-opt=stderr for inline messages. Yes, if we can figure out a good way to get this to work (i.e. only emit the optimized messages and not the rest of the dump messages). And unfortunately to get them all you need to specify -fdump-ipa-all-optimized -fdump-tree-all-optimized -fdump-rtl-all-optimized instead of just -fopt-info. Unless we can add -fdump-all-all-optimized. Having general support requires cleanup of all the old style if (dump_file) fprintf (dump_file, ...) instances to be: if (dump_enabled_p ()) dump_printf
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
Jakub Jelinek ja...@redhat.com wrote: On Fri, Aug 30, 2013 at 11:28:34AM -0400, Diego Novillo wrote: Well, it was a wrong decision then. For some smaller types writing manual marker might be a sensible thing, or for some extra complicated constructs. But here we're talking about the most simple struct hierarchy imaginable. Having to write manual markers for that one is absurd IMO. I want to discourage extending gengtype more than necessary. Long term, I would like to see memory pools replacing GC. However, that is likely a long road so we should find an interim solution. I have doubts that, still somewhat remember the obstack era and memory pools would again bring all the issues back, but let's put that aside. I vaguely remember thinking about what would be needed to have gengtype deal with inheritance. It needed some pretty ugly annotations. This made gengtype even more magic. That's very bad for maintenance. Teaching the gengtype parser about {struct,class} name : public {struct,class} someothername { ... } as opposed to current {struct,class} name { ... } shouldn't be that hard. And, if the complaint is that we'd need to write whole C++ parser for it, then the response could be that we already have one C++ parser (and, even have plugin support and/or emit dwarf etc.). Note that explicit markings was also about simply writing GTY((derivesfrom(xxx))) instead of extending the parser. The result could be as simple as gengtype emitting ggc_mx ((xxx) *this) In the marker routine. Richard. So, gengtype could very well use a g++ plugin to emit the stuff it needs, or parse DWARF, etc. And, we even could not require everybody actually emitting those themselves, we could check some text form of that (gengtype.state?) into the tree, so only people actually changing the compiler would need to run the plugin. One thing I liked is a suggestion that went something along the lines of creating some base templates that could be used to facilitate writing the manual markers. Even if you have some stuff that helps you writing those, still it will be a big source of bugs (very hard to debug) and a maintainance nightmare. Jakub
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
Diego Novillo dnovi...@google.com wrote: On Fri, Aug 30, 2013 at 11:37 AM, Jakub Jelinek ja...@redhat.com wrote: Teaching the gengtype parser about {struct,class} name : public {struct,class} someothername { ... } as opposed to current {struct,class} name { ... } shouldn't be that hard. And, if the complaint is that we'd need to write whole C++ parser for it, then the response could be that we already have one C++ parser (and, even have plugin support and/or emit dwarf etc.). It isn't. It's annoying and a duplication of effort. So, gengtype could very well use a g++ plugin to emit the stuff it needs, or parse DWARF, etc. And, we even could not require everybody actually emitting those themselves, we could check some text form of that (gengtype.state?) into the tree, so only people actually changing the compiler would need to run the plugin. Yes. Lawrence and I thought about moving gengtype inside g++. That seemed like a promising approach. What do you do during stage1? Have a collector that never collects? Richard. Even if you have some stuff that helps you writing those, still it will be a big source of bugs (very hard to debug) and a maintainance nightmare. Debugging gengtype is much harder. It is magic code that is not visible. Diego.
Re: [Patch] Rewrite regex matchers
Hi, stephen.w...@bregmasoft.ca wrote: Other than the test case (breaks on mingw32 target) this is looking very sweet. Could you please show some details? I suppose he referred to the char/wchar_t split which I described... Thanks! Paolo
Re: Fix OBJ_TYPE_REF handling in ipa-cp
Bootstrapped/regtesed x86_64-linux. Martin, please can you review the change? * ipa-prop.c (ipa_set_jf_known_type): Check that component type is a record type with BINFO. (detect_type_change_ssa): Add comp_type argument. (compute_complex_assign_jump_func): Add param_type argument; pass it down to detect_type_change_ssa. (compute_known_type_jump_func): Add expected_type parameter. Do not bother tracking a non-polymorphic type. (ipa_get_callee_param_type): New function. (ipa_compute_jump_functions_for_edge): Pass down calle parm types. (ipa_analyze_virtual_call_uses): Use class typee as argument of detect_type_change_1. (ipa_intraprocedural_devirtualization): Pass down class type. Hopefully I'll get rid of the component_types in the jump functions and most of this won't be necesary. Meanwhile, this is OK. Indeed, I hope these will go. I updated the patch to tree after your changes and fixed the remaining uses of detect_type_change so detect_type_change_1 can go completely. Bootstrapped/regtested x86_64-linux and also tested with Firefox build, comitted. Interestingly, we now have smaller static counts of devirtualization on firefox than two weeks ago. It is not caused by this patch however. While looking for possible causes I also noticed we don't seem to handle invisible references well. Consider: class A {virtual void b(void) { };}; class B : public A {virtual void b(void) { };}; void q (class A a); void qq (class A *a); void t (class A a) { q(a); qq(a); } void m() { class B b; t(b); } With -O2 -fno-early-inling -flto I get: m() () { struct B b; struct A D.2267; bb 2: B::B (b); A::A (D.2267, b.D.2212); t (D.2267); D.2267 ={v} {CLOBBER}; b ={v} {CLOBBER}; return; } t(A) (struct A restrict a) { struct A D.2242; bb 2: A::A (D.2242, a_2(D)); q (D.2242); D.2242 ={v} {CLOBBER}; qq (a_2(D)); return; } I.e. t is called with invisible reference. Now we get: Jump functions: Jump functions of caller m()/12: callsite m()/12 - t(A)/5 : param 0: UNKNOWN callsite m()/12 - A::A(A const)/4 : param 0: KNOWN TYPE: base struct A, offset 0, component struct A param 1: KNOWN TYPE: base struct B, offset 0, component const struct A callsite m()/12 - B::B()/11 : param 0: KNOWN TYPE: base struct B, offset 0, component struct B Jump functions of caller t(A)/5: callsite t(A)/5 - qq(A*)/20 : param 0: PASS THROUGH: 0, op nop_expr, type_preserved callsite t(A)/5 - q(A)/19 : param 0: UNKNOWN callsite t(A)/5 - A::A(A const)/4 : param 0: KNOWN TYPE: base struct A, offset 0, component struct A param 1: PASS THROUGH: 0, op nop_expr, type_preserved I think m()-t() has no type and t-qq is pass thorugh. So we miss the fact that t-qq is known type of A. Do we need jump function that is PASS THROGH KNOWN_TYPE at once? Honza * ipa-prop.c (ipa_set_jf_known_type): Check that we add only records. (detect_type_change_1): Rename to ... (detect_type_change): ... this one; early return on non-polymorphic types. (detect_type_change_ssa): Add comp_type parameter; update use of detect_type_change. (compute_complex_assign_jump_func): Add param_type parameter; update use of detect_type_change_ssa. (compute_complex_ancestor_jump_func): Likewise. (ipa_get_callee_param_type): New function. (ipa_compute_jump_functions_for_edge): Compute parameter type; update calls to the jump function computation functions. Index: ipa-prop.c === --- ipa-prop.c (revision 202092) +++ ipa-prop.c (working copy) @@ -371,6 +371,8 @@ static void ipa_set_jf_known_type (struct ipa_jump_func *jfunc, HOST_WIDE_INT offset, tree base_type, tree component_type) { + gcc_assert (TREE_CODE (component_type) == RECORD_TYPE + TYPE_BINFO (component_type)); jfunc-type = IPA_JF_KNOWN_TYPE; jfunc-value.known_type.offset = offset, jfunc-value.known_type.base_type = base_type; @@ -633,13 +635,16 @@ check_stmt_for_type_change (ao_ref *ao A -/* Like detect_type_change but with extra argument COMP_TYPE which will become - the component type part of new JFUNC of dynamic type change is detected and - the new base type is identified. */ +/* Detect whether the dynamic type of ARG of COMP_TYPE has changed (before + callsite CALL) by looking for assignments to its virtual table pointer. If + it is, return true and fill in the jump function JFUNC with relevant type + information or set it to unknown. ARG is the object itself (not a pointer + to it, unless dereferenced). BASE is the base of the memory access as + returned by get_ref_base_and_extent, as is the offset. */ static bool -detect_type_change_1 (tree arg, tree base,
[PATCH, alpha]: Fix recent gfortran.dg/pr32533.f90 test failure
Hello! The compilation emitted following sequence for cmove with unsigned compare, resulting in gfortran.dg/pr32533.f90 runtime failure [1]: (insn 70 69 71 (set (reg:DI 143) (leu:DI (reg:DI 139) (const_int 18 [0x12]))) -1 (nil)) (insn 71 70 72 (set (reg:DF 144) (eq:DF (subreg:DF (reg:DI 143) 0) (const_double:DF 0.0 [0x0.0p+0]))) -1 (nil)) (insn 72 71 73 (set (reg:DF 137 [ D.934 ]) (if_then_else:DF (eq (reg:DF 144) (const_double:DF 0.0 [0x0.0p+0])) (reg:DF 137 [ D.934 ]) (reg:DF 140))) -1 (nil)) where (insn 71) trapped with denormal operand FP exception. The problem was in alpha_emit_conditional_move, where fixup code didn't trigger for code variable, changed in if (FLOAT_MODE_P (cmp_mode) != FLOAT_MODE_P (mode)) part. Since cmove insns don't trap on compare, the compare of (insn 71) should be put inside cmove itself. Attached patch updates cmp RTX for code changes, resulting in: (insn 70 69 71 (set (reg:DI 143) (leu:DI (reg:DI 139) (const_int 18 [0x12]))) -1 (nil)) (insn 71 70 72 (set (reg:DF 137 [ D.934 ]) (if_then_else:DF (ne (subreg:DF (reg:DI 143) 0) (const_double:DF 0.0 [0x0.0p+0])) (reg:DF 137 [ D.934 ]) (reg:DF 140))) -1 (nil)) 2013-08-31 Uros Bizjak ubiz...@gmail.com * config/alpha/alpha.c (alpha_emit_conditional_move): Update cmp RTX before signed_comparison_operator check to account for code changes. Patch was tested on alphaev68-pc-linux-gnu and committed to mainline. [1] http://gcc.gnu.org/ml/gcc-testresults/2013-08/msg02997.html Uros. Index: config/alpha/alpha.c === --- config/alpha/alpha.c(revision 202125) +++ config/alpha/alpha.c(working copy) @@ -2659,6 +2659,7 @@ alpha_emit_conditional_move (rtx cmp, enum machine cmp_mode = cmp_mode == DImode ? DFmode : DImode; op0 = gen_lowpart (cmp_mode, tem); op1 = CONST0_RTX (cmp_mode); + cmp = gen_rtx_fmt_ee (code, VOIDmode, op0, op1); local_fast_math = 1; }
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Sat, 2013-08-31 at 11:57 +0200, Richard Biener wrote: Diego Novillo dnovi...@google.com wrote: Yes. Lawrence and I thought about moving gengtype inside g++. That seemed like a promising approach. What do you do during stage1? Have a collector that never collects? We could imagine that the successor of gengtype would be some GCC plugin (which would generate C++ code for marking and GC and PCH purposes, perhaps using ad-hoc attributes and pragmas) Then for bootstrapping purposes, we could put the generated C++ code in the source repository (like we already do for configure, or fixincludes/fixincl.x etc...). Hence stage1 would be buildable with the generated C++ code in the repository. A more difficult issue is that the set of GTY-ed types is target specific and depends upon the .../configure argument at build time. Perhaps we could consider processing all of it (i.e. every GTY-ed class declaration), and have our gengtype successor plugin emit appropriate #if in the generated C++ code. Of course having gengtype replaced by a plugin requires such a plugin to be developed and GCC maintainers to have access to some gcc... Cheers -- Basile STARYNKEVITCH http://starynkevitch.net/Basile/ email: basileatstarynkevitchdotnet mobile: +33 6 8501 2359 8, rue de la Faiencerie, 92340 Bourg La Reine, France *** opinions {are only mine, sont seulement les miennes} ***
[PATCH 3/6 v2] Autogenerated conversion of gimple to C++
On Thu, 2013-08-29 at 12:20 -0400, David Malcolm wrote: This patch is 110KB in size, so to avoid mailing-list size limits I've uploaded it to: http://dmalcolm.fedorapeople.org/gcc/large-patches/a89d361b4f95dd216e1d29cb44fbaf90372c48b8-0003-Autogenerated-conversion-of-gimple-to-C.patch The ChangeLog entry and diffstat follow: gcc/ Patch autogenerated by refactor_gimple.py from https://github.com/davidmalcolm/gcc-refactoring-scripts revision 26aabff11750d29e6129930a426242a564538d1a I've fixed the bug with OMP_TASK mentioned in http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01934.html by regenerating this patch with a fixed refactor_gimple.py [1] Again, it's 110KB in size, exceeding the list limit, so I've uploaded it here: http://dmalcolm.fedorapeople.org/gcc/large-patches/733a06e20496464e373c1f48388cedf0b6935a4e-0003-Autogenerated-conversion-of-gimple-to-C.patch The only effective difference between the patches is this: +inline bool +is_a_helper gimple_statement_omp_parallel::test (gimple gs) +{ -+ return gs-code == GIMPLE_OMP_PARALLEL; ++ return gs-code == GIMPLE_OMP_PARALLEL || gs-code == GIMPLE_OMP_TASK; +} + (and the const equivalent). Successfully bootstrapped on x86_64-unknown-linux-gnu with plain configure and thus with checking enabled; all tests show same results as a unpatched control build (of r202029), fixing the omp ICEs described above. Dave [1] Specifically, this commit to my refactoring scripts: https://github.com/davidmalcolm/gcc-refactoring-scripts/commit/d55c7a3e3d1227bad36284ffdb67c83fb089b272
Fix integer overflow when scaling counts in inliner
Hi, PPC64 lto profiledbootstrap failes because counts get negative in iteger scalling. Because of lost comdat samples, we end up with function called twice being inlinined into an call happening few thousdand times. gcov_scale is then large and apply_scale should not truncate it to 32bit. Bootstrapped/regtested x86_64-linux, comitted. Honza Index: ChangeLog === --- ChangeLog (revision 202127) +++ ChangeLog (working copy) @@ -1,3 +1,7 @@ +2013-08-31 Jan Hubicka j...@suse.cz + + * basic-block.h (apply_scale): Make scale parmeter gcov_type. + 2013-08-31 Uros Bizjak ubiz...@gmail.com * config/alpha/alpha.c (alpha_emit_conditional_move): Update Index: basic-block.h === --- basic-block.h (revision 202100) +++ basic-block.h (working copy) @@ -958,7 +958,7 @@ combine_probabilities (int prob1, int pr constrained to be REG_BR_PROB_BASE. */ static inline gcov_type -apply_scale (gcov_type freq, int scale) +apply_scale (gcov_type freq, gcov_type scale) { return RDIV (freq * scale, REG_BR_PROB_BASE); }
[PATCH 6/6 v2] Add manual GTY hooks
On Fri, 2013-08-30 at 10:04 +0200, Richard Biener wrote: On Thu, Aug 29, 2013 at 6:20 PM, David Malcolm dmalc...@redhat.com wrote: * gimple.c (gt_ggc_mx (gimple)): New, as required by GTY((user)). (gt_pch_nx (gimple)): Likewise. (gt_pch_nx (gimple, gt_pointer_operator, void *)): Likewise. * gimple.h (gt_ggc_mx (gimple)): Declare. (gt_pch_nx (gimple)): Declare. (gt_pch_nx (gimple, gt_pointer_operator, void *)): Declare. No GIMPLE should reside in PCHs so you should be able to just put gcc_unreachable () in them ... (if dropping them does not work) Thanks. I'm attaching a revised version of the patch which does this, bringing the length of the hand-written GTY code for this down from 741 lines to 267 lines. Successfully bootstrapped on x86_64-unknown-linux-gnu (with plain configure and thus with checking enabled); all tests show same results as a unpatched control build (of r202029). From 4edea4c28c624d4d308d8be731f4415d22e10f14 Mon Sep 17 00:00:00 2001 From: David Malcolm dmalc...@redhat.com Date: Mon, 26 Aug 2013 20:41:04 -0400 Subject: [PATCH 6/6] Add manual GTY hooks * gimple.c (gt_ggc_mx (gimple)): New, as required by GTY((user)). (gt_pch_nx (gimple)): New, stub implementation. (gt_pch_nx (gimple, gt_pointer_operator, void *)): Likewise. * gimple.h (gt_ggc_mx (gimple)): Declare. (gt_pch_nx (gimple)): Declare. (gt_pch_nx (gimple, gt_pointer_operator, void *)): Declare. * tree-cfg.c (ggc_mx (gimple)): Remove declaration, as this collides with the function that GTY((user)) expects. (gt_ggc_mx (edge_def *)): Replace call to gt_ggc_mx on the gimple with gt_ggc_mx_gimple_statement_base: in the pre-GTY((user)) world, gt_ggc_mx was the function to be called on a possibly NULL pointed to check if needed marking and if so to traverse its fields. In the GTY((user)) world, gt_ggc_mx is the function to be called on non-NULL objects immediately *after* they have been marked: it does not mark the object itself. (gt_pch_nx (gimple)): Remove declaration. (gt_pch_nx (edge_def *)): Update as per the mx hook. --- gcc/gimple.c | 269 + gcc/gimple.h | 6 ++ gcc/tree-cfg.c | 6 +- 3 files changed, 277 insertions(+), 4 deletions(-) diff --git a/gcc/gimple.c b/gcc/gimple.c index 1ad36d1..a5b4799 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -4338,4 +4338,273 @@ build_type_cast (tree to_type, gimple op, enum ssa_mode mode) return build_type_cast (to_type, gimple_assign_lhs (op), mode); } +void +gt_ggc_mx (gimple gs) +{ + gimple x = gs; + /* Emulation of the chain_next GTY attribute. + + gs has already been marked. + Iterate the chain of next statements, marking until we reach one that + has already been marked, or NULL. */ + gimple xlimit = gs-next; + while (ggc_test_and_set_mark (xlimit)) +xlimit = xlimit-next; + + /* All of the statements within the half-open interval [x..xlimit) have + just been marked. Iterate through the list, visiting their fields. */ + while (x != xlimit) +{ + gt_ggc_m_15basic_block_def (x-bb); + switch (gimple_statement_structure (((*x + { + case GSS_BASE: + break; + case GSS_WITH_OPS: + { + gimple_statement_with_ops *stmt + = static_cast gimple_statement_with_ops * (x); + size_t num = (size_t)(stmt-num_ops); + for (size_t i = 0; i != num; i++) + gt_ggc_m_9tree_node (stmt-op[i]); + } + break; + case GSS_WITH_MEM_OPS_BASE: + break; + case GSS_WITH_MEM_OPS: + { + gimple_statement_with_memory_ops *stmt + = static_cast gimple_statement_with_memory_ops * (x); + size_t num = (size_t)(stmt-num_ops); + for (size_t i = 0; i != num; i++) + gt_ggc_m_9tree_node (stmt-op[i]); + } + break; + case GSS_CALL: + { + gimple_statement_call *stmt + = static_cast gimple_statement_call * (x); + gt_ggc_m_15bitmap_head_def (stmt-call_used.vars); + gt_ggc_m_15bitmap_head_def (stmt-call_clobbered.vars); + switch (stmt-subcode GF_CALL_INTERNAL) + { + case 0: + gt_ggc_m_9tree_node (stmt-u.fntype); + break; + case GF_CALL_INTERNAL: + break; + default: + break; + } + size_t num = (size_t)(stmt-num_ops); + for (size_t i = 0; i != num; i++) + gt_ggc_m_9tree_node (stmt-op[i]); + } + break; + case GSS_OMP: + { + gimple_statement_omp *stmt + = static_cast gimple_statement_omp * (x); + gt_ggc_mx_gimple_statement_base (stmt-body); + } + break; + case GSS_BIND: + { + gimple_statement_bind *stmt + = static_cast gimple_statement_bind * (x); + gt_ggc_m_9tree_node (stmt-vars); + gt_ggc_m_9tree_node (stmt-block); + gt_ggc_mx_gimple_statement_base (stmt-body); + } + break; + case GSS_CATCH: + { + gimple_statement_catch *stmt + = static_cast gimple_statement_catch * (x); + gt_ggc_m_9tree_node (stmt-types); +
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Sat, Aug 31, 2013 at 5:57 AM, Richard Biener richard.guent...@gmail.com wrote: What do you do during stage1? Have a collector that never collects? Yes. That was the pebble in the shoe. The cc1plus built for the purposes of gengtype does not need to look at a lot of code, so turning off collection may not be a big problem. We also considered using a retargetable parser like Clang, or even plugins. All these approaches meant keeping GC. Neither of us is very fond of GC, so we did not explore these alternatives very deeply. Diego.
Re: [PATCH] Convert more passes to new dump framework
On Sat, Aug 31, 2013 at 12:26 AM, Bernhard Reutner-Fischer rep.dot@gmail.com wrote: On 30 August 2013 23:23:16 Teresa Johnson tejohn...@google.com wrote: On Fri, Aug 30, 2013 at 1:30 PM, Xinliang David Li davi...@google.com wrote: On Fri, Aug 30, 2013 at 12:51 PM, Teresa Johnson tejohn...@google.com wrote: On Fri, Aug 30, 2013 at 9:27 AM, Xinliang David Li davi...@google.com wrote: Except that in this form, the dump will be extremely large and not suitable for very large applications. Yes. I did some measurements for both a fairly large source file that is heavily optimized with LIPO and for a simple toy example that has some inlining. For the large source file, the output from -fdump-ipa-inline=stderr was almost 100x the line count of the -fopt-info output. For the toy source file it was 43x. The size of the -details output was 250x and 100x, respectively. Which is untenable for a large app. The issue I am having here is that I want a more verbose message, not a more voluminous set of messages. Using either -fopt-info-all or -fdump-ipa-inline to provoke the more verbose inline message will give me a much greater volume of output. One compromise could be to emit the more verbose inliner message under a param (and a more concise foo inlined into bar by default with -fopt-info). Or we could do some variant of what David talks about below. something like --param=verbose-opt-info=1 Yes. Richard, would this be acceptable for now? i.e. the inliner messages would be like: -fopt-info: test.c:8:3: note: foobar inlined into foo with call count 9000 (the with call count X only when there is profile feedback) -fopt-info --param=verbose-opt-info=1: test.c:8:3: note: foobar/0 (9000) inlined into foo/2 (1000) with call count 9000 (via inline instance bar [3] (9000)) (again the call counts only emitted under profile feedback) Assuming the [3] is order, please change that to match what the in liner uses, I.e. /3 Agreed - I meant to switch that back to / in both places but missed the last. It should read: test.c:8:3: note: foobar/0 (9000) inlined into foo/2 (1000) with call count 9000 (via inline instance bar/3 (9000)) Thanks, Teresa Thanks Besides, we might also want to use the same machinery (dump_printf_loc etc) for dump file dumping. The current behavior of using '-details' to turn on opt-info-all messages for dump files are not desirable. Interestingly, this doesn't even work. When I do -fdump-ipa-inline-details=stderr (with my patch containing the inliner messages) I am not getting those inliner messages emitted to stderr. Even though in dumpfile.c details is set to (TDF_DETAILS | MSG_OPTIMIZED_LOCATIONS | MSG_MISSED_OPTIMIZATION | MSG_NOTE). I'm not sure why, but will need to debug this. It works for vectorizer pass. Ok, let me see what is going on - I just confirmed that it is not working for the loop unroller messages either. How about the following: 1) add a new dump_kind modifier so that when that modifier is specified, the messages won't goto the alt_dumpfile (controlled by -fopt-info), but only to primary dump file. With this, the inline messages can be dumped via: dump_printf_loc (OPT_OPTIMIZED_LOCATIONS | OPT_DUMP_FILE_ONLY, .) (you mean (MSG_OPTIMIZED_LOCATIONS | OPT_DUMP_FILE_ONLY) ) Yes. Typically OR-ing together flags like this indicates dump under any of those conditions. But we could implement special handling for OPT_DUMP_FILE_ONLY, which in the above case would mean dump only to the primary dump file, and only under the other conditions specified in the flag (here under -optimized) 2) add more flags in -fdump- support: -fdump-ipa-inline-opt -- turn on opt-info messages only -fdump-ipa-inline-optall -- turn on opt-info-all messages According to the documentation (see the -fdump-tree- documentation on http://gcc.gnu.org/onlinedocs/gcc/Debugging-Options.html#Debugging-Options), the above are already supposed to be there (-optimized, -missed, -note and -optall). However, specifying any of these gives a warning like: cc1: warning: ignoring unknown option ‘optimized’ in ‘-fdump-ipa-inline’ [enabled by default] Probably because none is listed in the dump_options[] array in dumpfile.c. However, I don't think there is currently a way to use -fdump- options and *only* get one of these, as much of the current dump output is emitted whenever there is a dump_file defined. Until everything is migrated to the new framework it may be difficult to get this to work. -fdump-tree-pre-ir -- turn on GIMPLE dump only -fdump-tree-pre-details -- turn on everything (ir, optall, trace) With this, developers can really just use -fdump-ipa-inline-opt=stderr for inline messages. Yes, if we can figure out a good way to get this to work (i.e. only
Fix speculative edge reference lookup
Hi, this patch fixes ugly thinko when looking up reference for a speculative call. Without LTO we can end up choosing wrong alternative for function with many devirtualizations (as it happens for PPC64) Bootstrapped/regtested ppc64-linux, comitted. Honza Index: ChangeLog === --- ChangeLog (revision 202128) +++ ChangeLog (working copy) @@ -1,5 +1,9 @@ 2013-08-31 Jan Hubicka j...@suse.cz + * cgraph.c (cgraph_speculative_call_info): Fix ref lookup + +2013-08-31 Jan Hubicka j...@suse.cz + * basic-block.h (apply_scale): Make scale parmeter gcov_type. 2013-08-31 Uros Bizjak ubiz...@gmail.com Index: cgraph.c === --- cgraph.c(revision 202100) +++ cgraph.c(working copy) @@ -1151,7 +1151,7 @@ cgraph_speculative_call_info (struct cgr i, ref); i++) if (ref-speculative ((ref-stmt ref-stmt == e-call_stmt) - || (ref-lto_stmt_uid == e-lto_stmt_uid))) + || (!ref-stmt ref-lto_stmt_uid == e-lto_stmt_uid))) { reference = ref; break;
Re: [PATCH 6/6] Add manual GTY hooks
On Fri, 2013-08-30 at 10:09 +0200, Richard Biener wrote: On Thu, Aug 29, 2013 at 9:44 PM, Steven Bosscher stevenb@gmail.com wrote: On Thu, Aug 29, 2013 at 6:20 PM, David Malcolm dmalc...@redhat.com wrote: * gimple.c (gt_ggc_mx (gimple)): New, as required by GTY((user)). (gt_pch_nx (gimple)): Likewise. GIMPLE isn't supposed to end up in a PCH. Can you please make this function simply call gcc_unreachable()? FWIW 1: I really think all these hand-written markers aren't a good idea, we should really figure out a way to have automatic marker function generators, something less complex than gengtype, of course. But to have all these calls to the type-mangled marker functions (gt_pch_n_9tree_node, etc.) is going to be a problem in the long term. It seems to me that the first step in all these conversions to hand-written markers should be to make gengtype spit out the marker functions *without* the type name mangling, i.e. all marker functions should just be gt_ggc_mx(type) / gt_pch_nx(type). Yes, the original idea was that gengtype would do that. For things we like to optimize the GTY((user)) thing would tell it that we do provide the markers. Like when you want to look through a non-GCed intermediate object. Or for things like GTY((chain)) stuff that doesn't really work if you have multiple chains (without clever GTY((skip))s...). The lack of the unmangled overloads is annoying :/ IIRC Diego halfway completed the transition to unmangled overloads / specializations? How would that work, and is that something that it would be productive for me to work on? Currently AIUI gtype-desc.h contains mangled macros and decls e.g.: extern void gt_ggc_mx_rtvec_def (void *); #define gt_ggc_m_9rtvec_def(X) do { \ if (X != NULL) gt_ggc_mx_rtvec_def (X);\ } while (0) and the corresponding functions in gtype-desc.c contain: void gt_ggc_mx_rtvec_def (void *x_p) { struct rtvec_def * const x = (struct rtvec_def *)x_p; if (ggc_test_and_set_mark (x)) { /* visit fields of x, invoking the macros */ } } Is the goal for the field-visiting code to all be able to simply do: gt_ggc_mx (field) and have overloading resolve it all? (and handle e.g. chain_next etc etc) Presumably if this were implemented, then hand-written GTY functions would be that much easier to maintain, and perhaps this gimple conversion idea would be more acceptable? (or, in other words, should I work on this?) Thanks Dave
Fix fork instrumentation for libgcov
Hi, as noticed by Martin Liska, -O0 -fprofile-generate code won't land into __gcov_fork. This is due to early exit from expand_builtin. Fixed and will be comitted as obvious. Honza * builtins.c (expand_builtin): Do not exit early for gcov instrumented functions. * gcc.dg/fork-instrumentation.c: New testcase. Index: builtins.c === --- builtins.c (revision 202100) +++ builtins.c (working copy) @@ -5850,6 +5850,13 @@ expand_builtin (tree exp, rtx target, rt set of builtins. */ if (!optimize !called_as_built_in (fndecl) + fcode != BUILT_IN_FORK + fcode != BUILT_IN_EXECL + fcode != BUILT_IN_EXECV + fcode != BUILT_IN_EXECLP + fcode != BUILT_IN_EXECLE + fcode != BUILT_IN_EXECVP + fcode != BUILT_IN_EXECVE fcode != BUILT_IN_ALLOCA fcode != BUILT_IN_ALLOCA_WITH_ALIGN fcode != BUILT_IN_FREE) Index: testsuite/gcc.dg/fork-instrumentation.c === --- testsuite/gcc.dg/fork-instrumentation.c (revision 0) +++ testsuite/gcc.dg/fork-instrumentation.c (working copy) @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options -O0 -fprofile-generate } */ +int fork(void); +t() +{ + fork (); +} +/* { dg-final { scan-assembler gcov_fork } } */
Re: Ubsan merged into trunk
Hi, On 30 Aug 2013, at 20:43, Jakub Jelinek wrote: On Fri, Aug 30, 2013 at 09:38:01PM +0200, Dominique Dhumieres wrote: I've just merged ubsan into trunk. Please send complaints my way. Bootstrap is broken on x86_64-apple-darwin10: (wonder why not libasan_la_LIBADD = $(top_builddir)/sanitizer_common/libsanitizer_common.la if !USING_MAC_INTERPOSE libasan_la_LIBADD += $(top_builddir)/interception/libinterception.la endif libasan_la_LIBADD += $(LIBSTDCXX_RAW_CXX_LDFLAGS) ). … indeed, that's what I did for ubsan. perhaps tsan/Makefile.am too (though, tsan isn't supported on darwin, so it doesn't matter that much). tsan isn't relevant (yet): although, when we get some time to work on it, native thread support should be feasible for Darwin = 11. === the patch below fixes bootstrap - along the lines of your observation; it also fixes the specs to actually use the library (so that the tests pass too). bootstrapped x86_64-darwin12 for c,c++ and fortran (objc and ada bootstraps are broken from other causes). [also bootstrapped on x86_64-linux, and checked RUNTESTFLAGS=asan.exp ubsan.exp] OK for trunk? Iain gcc: * config/darwin.h (LINK_COMMAND_SPEC_A): Revise sanitiser specs to include sanitise(undefined). libsanitiser: * ubsan/Makefile.am (libubsan_la_LIBADD): Revise to omit libinterception.la for Darwin. * ubsan/Makefile.in: Regenerate. Index: gcc/config/darwin.h === --- gcc/config/darwin.h (revision 202118) +++ gcc/config/darwin.h (working copy) @@ -178,10 +178,11 @@ extern GTY(()) int darwin_ms_struct; %{L*} %(link_libgcc) %o %{fprofile-arcs|fprofile-generate*|coverage:-lgcov} \ %{fopenmp|ftree-parallelize-loops=*: \ %{static|static-libgcc|static-libstdc++|static-libgfortran: libgomp.a%s; : -lgomp } } \ -%{%:sanitize(address): -lasan } \ %{fgnu-tm: \ %{static|static-libgcc|static-libstdc++|static-libgfortran: libitm.a%s; : -litm } } \ %{!nostdlib:%{!nodefaultlibs:\ + %{%:sanitize(address): -lasan } \ + %{%:sanitize(undefined): -lubsan } \ %(link_ssp) %(link_gcc_c_sequence)\ }}\ %{!nostdlib:%{!nostartfiles:%E}} %{T*} %{F*} }}} Index: libsanitizer/ubsan/Makefile.am === --- libsanitizer/ubsan/Makefile.am (revision 202118) +++ libsanitizer/ubsan/Makefile.am (working copy) @@ -18,7 +18,11 @@ ubsan_files = \ ubsan_value.cc libubsan_la_SOURCES = $(ubsan_files) -libubsan_la_LIBADD = $(top_builddir)/sanitizer_common/libsanitizer_common.la $(top_builddir)/interception/libinterception.la $(LIBSTDCXX_RAW_CXX_LDFLAGS) +libubsan_la_LIBADD = $(top_builddir)/sanitizer_common/libsanitizer_common.la +if !USING_MAC_INTERPOSE +libubsan_la_LIBADD += $(top_builddir)/interception/libinterception.la +endif +libubsan_la_LIBADD += $(LIBSTDCXX_RAW_CXX_LDFLAGS) libubsan_la_LDFLAGS = -version-info `grep -v '^\#' $(srcdir)/libtool-version` -lpthread -ldl # Work around what appears to be a GNU make bug handling MAKEFLAGS
Re: Ubsan merged into trunk
On Sat, Aug 31, 2013 at 04:04:03PM +0100, Iain Sandoe wrote: Hi, On 30 Aug 2013, at 20:43, Jakub Jelinek wrote: On Fri, Aug 30, 2013 at 09:38:01PM +0200, Dominique Dhumieres wrote: I've just merged ubsan into trunk. Please send complaints my way. Bootstrap is broken on x86_64-apple-darwin10: (wonder why not libasan_la_LIBADD = $(top_builddir)/sanitizer_common/libsanitizer_common.la if !USING_MAC_INTERPOSE libasan_la_LIBADD += $(top_builddir)/interception/libinterception.la endif libasan_la_LIBADD += $(LIBSTDCXX_RAW_CXX_LDFLAGS) ). … indeed, that's what I did for ubsan. perhaps tsan/Makefile.am too (though, tsan isn't supported on darwin, so it doesn't matter that much). tsan isn't relevant (yet): although, when we get some time to work on it, native thread support should be feasible for Darwin = 11. === the patch below fixes bootstrap - along the lines of your observation; it also fixes the specs to actually use the library (so that the tests pass too). bootstrapped x86_64-darwin12 for c,c++ and fortran (objc and ada bootstraps are broken from other causes). [also bootstrapped on x86_64-linux, and checked RUNTESTFLAGS=asan.exp ubsan.exp] OK for trunk? Iain I was just about to post something similar; thanks for the patch and sorry for the breakage. Can't give you formal approval though. gcc: * config/darwin.h (LINK_COMMAND_SPEC_A): Revise sanitiser specs to include sanitise(undefined). s/sanitise/sanitize/ ;) Marek
Re: Ubsan merged into trunk
On Fri, Aug 30, 2013 at 06:58:06PM -0400, David Edelsohn wrote: This patch / merge broke bootstrap on AIX: In file included from ./tm.h:18:0, from /home/dje/src/src/gcc/function.h:26, from /home/dje/src/src/gcc/basic-block.h:25, from /home/dje/src/src/gcc/cgraph.h:28, from /home/dje/src/src/gcc/ubsan.c:25: /home/dje/src/src/gcc/ubsan.c: In function 'tree_node* ubsan_type_descriptor(tree)': /home/dje/src/src/gcc/config/rs6000/xcoff.h:262:63: error: 'rs6000_xcoff_strip_dollar' was not declared in this scope sprintf (LABEL, *%s..%u, rs6000_xcoff_strip_dollar (PREFIX), (unsigned) (NUM)) ^ /home/dje/src/src/gcc/ubsan.c:282:3: note: in expansion of macro 'ASM_GENERATE_INTERNAL_LABEL' ASM_GENERATE_INTERNAL_LABEL (tmp_name, Lubsan_type, type_var_id_num++); ^ /home/dje/src/src/gcc/ubsan.c: In function 'tree_node* ubsan_create_data(const char*, location_t, ...)': /home/dje/src/src/gcc/config/rs6000/xcoff.h:262:63: error: 'rs6000_xcoff_strip_dollar' was not declared in this scope sprintf (LABEL, *%s..%u, rs6000_xcoff_strip_dollar (PREFIX), (unsigned) (NUM)) If you use macros like ASM_GENERATE_INTERNAL_LABEL, which may cal target-specific functions, you need to include tm_p.h to pull in target-protos.h. I see, sorry. Will commit the following fix as obvious in a bit. 2013-08-31 Marek Polacek pola...@redhat.com * ubsan.c: Include tm_p.h. --- gcc/ubsan.c.mp 2013-08-31 17:12:48.719219402 +0200 +++ gcc/ubsan.c 2013-08-31 17:13:05.895281454 +0200 @@ -27,6 +27,7 @@ along with GCC; see the file COPYING3. #include hashtab.h #include pointer-set.h #include output.h +#include tm_p.h #include toplev.h #include ubsan.h #include c-family/c-common.h Marek
Re: [PATCH] Sanitize block partitioning under -freorder-blocks-and-partition
Hi, With Martin we made script for testing the profiling failures. First do ld --verbose ~/script then apply --- /home/jh/script22013-08-31 17:59:11.0 +0200 +++ /home/jh/script 2013-08-31 17:39:40.0 +0200 @@ -1,12 +1,3 @@ -GNU ld (GNU Binutils for Debian) 2.20.1-system.20100303 - Supported emulations: - elf_x86_64 - elf_i386 - i386linux - elf_l1om -using internal linker script: -== -/* Script for -z combreloc: combine and sort reloc sections */ OUTPUT_FORMAT(elf64-x86-64, elf64-x86-64, elf64-x86-64) OUTPUT_ARCH(i386:x86-64) @@ -55,6 +46,7 @@ KEEP (*(.init)) } =0x90909090 .plt: { *(.plt) *(.iplt) } + .text.unlikely (NOLOAD) : { *(.text.unlikely .text.*_unlikely .text.unlikely.*) } .text : { *(.text.unlikely .text.*_unlikely) @@ -218,4 +210,3 @@ } -== then create t.c as: __attribute__ ((noinline)) t() { printf (test\n); } main(int argc) { if (argc1) t(); return 0; } and dotests as: for name in $* do rm a.out *.gcda 2/dev/null ./xgcc -B ./ -Ofast -fprofile-generate $name --static 2/dev/null if [ -f a.out ] then ./a.out /dev/null 2/dev/null || continue ./xgcc -B ./ -Ofast -fprofile-use -freorder-blocks-and-partition -Wl,-T,/home/jh/script --static $name 2/dev/null ./a.out t /dev/null 2/dev/null || echo FAIL $name else echo skip $name fi done Then run: jh@gcc10:~/trunk/build/gcc$ sh dotests t.c FAIL t.c You should get FAIL if things are fine, because t.c depends behaviour on number of command line parameters. If that fail you can run i.e. jh@gcc10:~/trunk/build/gcc$ sh dotests ~/trunk/gcc/testsuite/gcc.c-torture/execute/*.c skip /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/2402-1.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/2422-1.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/2910-2.c skip /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20010329-1.c Those with fail get cold section executed. When you run tehm again through dotests you can do gdb a.out and see what function gets split incorrectly. Honza
Remove hash from remember_with_vars
Hi, remember_with_vars walks trees that are read from file (now unique) and looks for fields that can contain pointers to vars or functions and if so, it records them to global hashtable for later fixup. This is quite wasteful, because the hash is querried many times. We can simply walk all fields in a tree and record the tree once afterwards, moreover since streaming knows when tree is newly constructed, we don't need hash. Vectors do not allow deletable elements, but I think it makes no difference: the trees are all used from decl states. * lot.c (tree_with_vars): Turn into vector. (MAYBE_REMEMBER_WITH_VARS): Change to... (CHECK_VAR): ... this one. (CHECK_NO_VAR): New macro. (maybe_remember_with_vars_typed): Trun to ... (mentions_vars_p_typed): ... this one. (maybe_remember_with_vars_common): Trun to ... (mentions_vars_p_comon): ... this one. (maybe_remember_with_vars_decl_minimal): Trun to ... (mentions_vars_p_decl_minmal): ... this one. (maybe_remember_with_vars_decl_common): Trun to ... (mentions_vars_p_decl_common): ... this one. (maybe_remember_with_vars_decl_with_vis): Trun to ... (mentions_vars_p_decl_with_vis): ... this one. (maybe_remember_with_vars_decl_non_common): Trun to ... (mentions_vars_p_decl_non_common): ... this one. (maybe_remember_with_vars_function): Trun to ... (mentions_vars_p_function): ... this one. (maybe_remember_with_vars_field_decl): Trun to ... (mentions_vars_p_field_decl): ... this one. (maybe_remember_with_vars_type): Trun to ... (mentions_vars_p_type): ... this one. (maybe_remember_with_vars_binfo): Trun to ... (mentions_vars_p_binfo): ... this one. (maybe_remember_with_vars_constructor): Trun to ... (mentions_vars_p_constructor): ... this one. (maybe_remember_with_vars_expr): Trun to ... (mentions_vars_p_expr): ... this one. (maybe_remember_with_vars): Trun to ... (mentions_vars_p): ... this one. (lto_read_decls): Update. (LTO_SET_PREVAIL): Do not call function for internal decls. (lto_fixup_prevailing_decls): Update to match mentions_vars_p; check that something was updated. (lto_fixup_state): Do not care about internal decls. (lto_fixup_decls): Update. (read_cgraph_and_symbols): Update. Index: lto/lto.c === --- lto/lto.c (revision 202099) +++ lto/lto.c (working copy) @@ -1283,196 +1286,206 @@ gimple_register_type (tree t) /* End of old merging code. */ +/* Remember trees that contains references to declarations. */ +static GTY(()) vec tree, va_gc *tree_with_vars; - -/* A hashtable of trees that potentially refer to variables or functions - that must be replaced with their prevailing variant. */ -static GTY((if_marked (ggc_marked_p), param_is (union tree_node))) htab_t - tree_with_vars; - -/* Remember that T is a tree that (potentially) refers to a variable - or function decl that may be replaced with its prevailing variant. */ -static void -remember_with_vars (tree t) -{ - *(tree *) htab_find_slot (tree_with_vars, t, INSERT) = t; -} - -#define MAYBE_REMEMBER_WITH_VARS(tt) \ +#define CHECK_VAR(tt) \ do \ { \ - if (tt) \ - { \ - if (VAR_OR_FUNCTION_DECL_P (tt) TREE_PUBLIC (tt)) \ - remember_with_vars (t); \ - } \ + if ((tt) VAR_OR_FUNCTION_DECL_P (tt) \ + (TREE_PUBLIC (tt) || DECL_EXTERNAL (tt))) \ + return true; \ } while (0) -/* Fix up fields of a tree_typed T. */ +#define CHECK_NO_VAR(tt) \ + gcc_checking_assert (!(tt) || !VAR_OR_FUNCTION_DECL_P (tt)) -static void -maybe_remember_with_vars_typed (tree t) +/* Check presence of pointers to decls in fields of a tree_typed T. */ + +static inline bool +mentions_vars_p_typed (tree t) { - MAYBE_REMEMBER_WITH_VARS (TREE_TYPE (t)); + CHECK_NO_VAR (TREE_TYPE (t)); + return false; } -/* Fix up fields of a tree_common T. */ +/* Check presence of pointers to decls in fields of a tree_common T. */ -static void -maybe_remember_with_vars_common (tree t) +static inline bool +mentions_vars_p_common (tree t) { - maybe_remember_with_vars_typed (t); - MAYBE_REMEMBER_WITH_VARS (TREE_CHAIN (t)); + if (mentions_vars_p_typed (t)) +return true; + CHECK_NO_VAR (TREE_CHAIN (t)); + return false; } -/* Fix up fields of a decl_minimal T. */ +/* Check presence of pointers to decls in fields of a decl_minimal T. */ -static void -maybe_remember_with_vars_decl_minimal (tree t) +static inline bool +mentions_vars_p_decl_minimal (tree t) { - maybe_remember_with_vars_common (t); - MAYBE_REMEMBER_WITH_VARS (DECL_NAME (t)); - MAYBE_REMEMBER_WITH_VARS (DECL_CONTEXT (t)); + if (mentions_vars_p_common (t)) +return true; + CHECK_NO_VAR (DECL_NAME (t)); + CHECK_VAR
Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
On Sat, 31 Aug 2013, Cong Hou wrote: I don't see why it would be unsafe for logb - can you give an example (exact float input value as hex float, and the values you believe logb should return for float and double). Please try the following code (you will get different results whether to use optimization mode): #include math.h #include stdio.h int main() { int i = 0x3edc67d5; float f = *((float*)i); float r1 = logb(f); float r2 = logbf(f); printf(%x %x\n, *((int*)r1), *((int*)r2)); } (a) Please stop sending HTML email, so your messages reach the mailing list, and resend your messages so far to the list. The mailing list needs to see the whole of both sides of the discussion of any patch being proposed for GCC. (b) I referred to the values *you believe logb should return*. Optimization is not meant to preserve library bugs; the comparison should be on the basis of correctly rounded results from both float and double functions. The correct return from logb appears to be -2 here, and I get that from both logb and logbf with current git glibc. The existence of a bug in some old library is not relevant here. (c) I always advise writing such tests as *valid C code* using hex floats (or if really necessary, unions), not *invalid C code* with aliasing violations. -- Joseph S. Myers jos...@codesourcery.com
Re: Remove hash from remember_with_vars
Jan Hubicka hubi...@ucw.cz wrote: Hi, remember_with_vars walks trees that are read from file (now unique) and looks for fields that can contain pointers to vars or functions and if so, it records them to global hashtable for later fixup. This is quite wasteful, because the hash is querried many times. We can simply walk all fields in a tree and record the tree once afterwards, moreover since streaming knows when tree is newly constructed, we don't need hash. Vectors do not allow deletable elements, but I think it makes no difference: the trees are all used from decl states. Heh, indeed a cleanup possibility I missed. Ok. Thanks, Richard. * lot.c (tree_with_vars): Turn into vector. (MAYBE_REMEMBER_WITH_VARS): Change to... (CHECK_VAR): ... this one. (CHECK_NO_VAR): New macro. (maybe_remember_with_vars_typed): Trun to ... (mentions_vars_p_typed): ... this one. (maybe_remember_with_vars_common): Trun to ... (mentions_vars_p_comon): ... this one. (maybe_remember_with_vars_decl_minimal): Trun to ... (mentions_vars_p_decl_minmal): ... this one. (maybe_remember_with_vars_decl_common): Trun to ... (mentions_vars_p_decl_common): ... this one. (maybe_remember_with_vars_decl_with_vis): Trun to ... (mentions_vars_p_decl_with_vis): ... this one. (maybe_remember_with_vars_decl_non_common): Trun to ... (mentions_vars_p_decl_non_common): ... this one. (maybe_remember_with_vars_function): Trun to ... (mentions_vars_p_function): ... this one. (maybe_remember_with_vars_field_decl): Trun to ... (mentions_vars_p_field_decl): ... this one. (maybe_remember_with_vars_type): Trun to ... (mentions_vars_p_type): ... this one. (maybe_remember_with_vars_binfo): Trun to ... (mentions_vars_p_binfo): ... this one. (maybe_remember_with_vars_constructor): Trun to ... (mentions_vars_p_constructor): ... this one. (maybe_remember_with_vars_expr): Trun to ... (mentions_vars_p_expr): ... this one. (maybe_remember_with_vars): Trun to ... (mentions_vars_p): ... this one. (lto_read_decls): Update. (LTO_SET_PREVAIL): Do not call function for internal decls. (lto_fixup_prevailing_decls): Update to match mentions_vars_p; check that something was updated. (lto_fixup_state): Do not care about internal decls. (lto_fixup_decls): Update. (read_cgraph_and_symbols): Update. Index: lto/lto.c === --- lto/lto.c (revision 202099) +++ lto/lto.c (working copy) @@ -1283,196 +1286,206 @@ gimple_register_type (tree t) /* End of old merging code. */ +/* Remember trees that contains references to declarations. */ +static GTY(()) vec tree, va_gc *tree_with_vars; - -/* A hashtable of trees that potentially refer to variables or functions - that must be replaced with their prevailing variant. */ -static GTY((if_marked (ggc_marked_p), param_is (union tree_node))) htab_t - tree_with_vars; - -/* Remember that T is a tree that (potentially) refers to a variable - or function decl that may be replaced with its prevailing variant. */ -static void -remember_with_vars (tree t) -{ - *(tree *) htab_find_slot (tree_with_vars, t, INSERT) = t; -} - -#define MAYBE_REMEMBER_WITH_VARS(tt) \ +#define CHECK_VAR(tt) \ do \ { \ - if (tt) \ - { \ -if (VAR_OR_FUNCTION_DECL_P (tt) TREE_PUBLIC (tt)) \ - remember_with_vars (t); \ - } \ + if ((tt) VAR_OR_FUNCTION_DECL_P (tt) \ + (TREE_PUBLIC (tt) || DECL_EXTERNAL (tt))) \ + return true; \ } while (0) -/* Fix up fields of a tree_typed T. */ +#define CHECK_NO_VAR(tt) \ + gcc_checking_assert (!(tt) || !VAR_OR_FUNCTION_DECL_P (tt)) -static void -maybe_remember_with_vars_typed (tree t) +/* Check presence of pointers to decls in fields of a tree_typed T. */ + +static inline bool +mentions_vars_p_typed (tree t) { - MAYBE_REMEMBER_WITH_VARS (TREE_TYPE (t)); + CHECK_NO_VAR (TREE_TYPE (t)); + return false; } -/* Fix up fields of a tree_common T. */ +/* Check presence of pointers to decls in fields of a tree_common T. */ -static void -maybe_remember_with_vars_common (tree t) +static inline bool +mentions_vars_p_common (tree t) { - maybe_remember_with_vars_typed (t); - MAYBE_REMEMBER_WITH_VARS (TREE_CHAIN (t)); + if (mentions_vars_p_typed (t)) +return true; + CHECK_NO_VAR (TREE_CHAIN (t)); + return false; } -/* Fix up fields of a decl_minimal T. */ +/* Check presence of pointers to decls in fields of a decl_minimal T. */ -static void -maybe_remember_with_vars_decl_minimal (tree t) +static inline bool +mentions_vars_p_decl_minimal (tree t) { - maybe_remember_with_vars_common (t); - MAYBE_REMEMBER_WITH_VARS (DECL_NAME (t)); - MAYBE_REMEMBER_WITH_VARS (DECL_CONTEXT (t)); + if (mentions_vars_p_common (t)) +return
Re: [PATCH 6/6] Add manual GTY hooks
David Malcolm dmalc...@redhat.com wrote: On Fri, 2013-08-30 at 10:09 +0200, Richard Biener wrote: On Thu, Aug 29, 2013 at 9:44 PM, Steven Bosscher stevenb@gmail.com wrote: On Thu, Aug 29, 2013 at 6:20 PM, David Malcolm dmalc...@redhat.com wrote: * gimple.c (gt_ggc_mx (gimple)): New, as required by GTY((user)). (gt_pch_nx (gimple)): Likewise. GIMPLE isn't supposed to end up in a PCH. Can you please make this function simply call gcc_unreachable()? FWIW 1: I really think all these hand-written markers aren't a good idea, we should really figure out a way to have automatic marker function generators, something less complex than gengtype, of course. But to have all these calls to the type-mangled marker functions (gt_pch_n_9tree_node, etc.) is going to be a problem in the long term. It seems to me that the first step in all these conversions to hand-written markers should be to make gengtype spit out the marker functions *without* the type name mangling, i.e. all marker functions should just be gt_ggc_mx(type) / gt_pch_nx(type). Yes, the original idea was that gengtype would do that. For things we like to optimize the GTY((user)) thing would tell it that we do provide the markers. Like when you want to look through a non-GCed intermediate object. Or for things like GTY((chain)) stuff that doesn't really work if you have multiple chains (without clever GTY((skip))s...). The lack of the unmangled overloads is annoying :/ IIRC Diego halfway completed the transition to unmangled overloads / specializations? How would that work, and is that something that it would be productive for me to work on? Currently AIUI gtype-desc.h contains mangled macros and decls e.g.: extern void gt_ggc_mx_rtvec_def (void *); #define gt_ggc_m_9rtvec_def(X) do { \ if (X != NULL) gt_ggc_mx_rtvec_def (X);\ } while (0) and the corresponding functions in gtype-desc.c contain: void gt_ggc_mx_rtvec_def (void *x_p) { struct rtvec_def * const x = (struct rtvec_def *)x_p; if (ggc_test_and_set_mark (x)) { /* visit fields of x, invoking the macros */ } } Is the goal for the field-visiting code to all be able to simply do: gt_ggc_mx (field) Yes. The advantage is that gengtype this way only needs to parse field names and not types which is a lot easier. and have overloading resolve it all? (and handle e.g. chain_next etc etc) Yes. All bits that would require more complex parsing should instead be telled explicit via a GTY annotation. Presumably if this were implemented, then hand-written GTY functions would be that much easier to maintain, and perhaps this gimple conversion idea would be more acceptable? (or, in other words, should I work on this?) That would be very nice! IIRC Diego had issues with making all declarations visible to make this work. Diego? Richard. Thanks Dave
Re: Remove hash from remember_with_vars
On 31 August 2013 19:15:46 Richard Biener rguent...@suse.de wrote: Jan Hubicka hubi...@ucw.cz wrote: Hi, remember_with_vars walks trees that are read from file (now unique) and looks for fields that can contain pointers to vars or functions and if so, it records them to global hashtable for later fixup. This is quite wasteful, because the hash is querried many times. We can simply walk all fields in a tree and record the tree once afterwards, moreover since streaming knows when tree is newly constructed, we don't need hash. Vectors do not allow deletable elements, but I think it makes no difference: the trees are all used from decl states. Heh, indeed a cleanup possibility I missed. Ok. s/Trun/Turn/g ? Thanks Thanks, Richard. * lot.c (tree_with_vars): Turn into vector. (MAYBE_REMEMBER_WITH_VARS): Change to... (CHECK_VAR): ... this one. (CHECK_NO_VAR): New macro. (maybe_remember_with_vars_typed): Trun to ... (mentions_vars_p_typed): ... this one. (maybe_remember_with_vars_common): Trun to ... (mentions_vars_p_comon): ... this one. (maybe_remember_with_vars_decl_minimal): Trun to ... (mentions_vars_p_decl_minmal): ... this one. (maybe_remember_with_vars_decl_common): Trun to ... (mentions_vars_p_decl_common): ... this one. (maybe_remember_with_vars_decl_with_vis): Trun to ... (mentions_vars_p_decl_with_vis): ... this one. (maybe_remember_with_vars_decl_non_common): Trun to ... (mentions_vars_p_decl_non_common): ... this one. (maybe_remember_with_vars_function): Trun to ... (mentions_vars_p_function): ... this one. (maybe_remember_with_vars_field_decl): Trun to ... (mentions_vars_p_field_decl): ... this one. (maybe_remember_with_vars_type): Trun to ... (mentions_vars_p_type): ... this one. (maybe_remember_with_vars_binfo): Trun to ... (mentions_vars_p_binfo): ... this one. (maybe_remember_with_vars_constructor): Trun to ... (mentions_vars_p_constructor): ... this one. (maybe_remember_with_vars_expr): Trun to ... (mentions_vars_p_expr): ... this one. (maybe_remember_with_vars): Trun to ... (mentions_vars_p): ... this one. (lto_read_decls): Update. (LTO_SET_PREVAIL): Do not call function for internal decls. (lto_fixup_prevailing_decls): Update to match mentions_vars_p; check that something was updated. (lto_fixup_state): Do not care about internal decls. (lto_fixup_decls): Update. (read_cgraph_and_symbols): Update. Sent with AquaMail for Android http://www.aqua-mail.com
Re: [patch, fortran, docs] Unformatted sequential and special files
On Fri, Aug 30, 2013 at 8:18 PM, Thomas Koenig tkoe...@netcologne.de wrote: Hello world, the attached patch documents the format for unformatted sequential files and what is, and is not, supported with special files. Tested with make dvi and make info. OK for trunk? The unformatted sequential format part looks Ok, but the special files section is lacking. E.g. what about - The REWIND statement? - The ENDFILE statement? - ACCESS='stream' and the POS= specifier? - ACCESS='direct'? (I suspect this should work for pipes/FIFO's/terminals in case REC= numbers are sequential, but is that a guarantee we want to make?) - Special files which are special in other ways. E.g. block special files tend to allow seeking, but IO must be block aligned. Thomas 2013-08-30 Thomas Koenig tkoe...@gcc.gnu.org PR fortran/30162 * gfortran.texi: Document unformatted sequential file format and I/O with special files. -- Janne Blomqvist
Re: Ubsan merged into trunk
bootstrapped x86_64-darwin12 for c,c++ and fortran ... Bootstrapped x86_64-apple-darwin10 for c,c++,fortran,java and also tested asan.exp and ubsan.exp for gcc and g++. Thanks for the patch, Dominique
Re: Ada PATCH: Fix ada/58239 by linking with xg++, not xgcc
This patch fixes that by introducing GXX_LINK which is GCC_LINK except that CXX (e.g. xg++) instead of CC (e.g. xgcc) is invoked. Eric, are there other executables that need to be linked with GXX_LINK too but aren't triggered yet? Yes, all not covered executables linking with TOOLS_LIBS since it contains libcommon.a which now drags the C++ library. So the simplest solution is to change GCC_LINK (there is one potential problematic case, vxaddr2line, but it probably didn't link before so let's forget it for now). I'll attach a patch to the PR so that the Darwin folks can test it. -- Eric Botcazou
Re: Ada PATCH: Fix ada/58239 by linking with xg++, not xgcc
Eric Botcazou ebotca...@adacore.com writes: | This patch fixes that by introducing GXX_LINK which is GCC_LINK except | that CXX (e.g. xg++) instead of CC (e.g. xgcc) is invoked. | | Eric, are there other executables that need to be linked with GXX_LINK | too but aren't triggered yet? | | Yes, all not covered executables linking with TOOLS_LIBS since it contains | libcommon.a which now drags the C++ library. So the simplest solution is to | change GCC_LINK (there is one potential problematic case, vxaddr2line, but it | probably didn't link before so let's forget it for now). | | I'll attach a patch to the PR so that the Darwin folks can test it. Thank you; that is very much appreciated. -- Gaby
Re: [PATCH] Sanitize block partitioning under -freorder-blocks-and-partition
Hi, I run my script on execute testsuite and looked into few testcases. The problem I found was roundoff errors - i.e. when expanding switch we set 50% chage that out of bound value is above or bellow. Both gets rounded to 0, because switch is executed once and the value is bellow. Partly this can be fixed by making probably_never_executed to consider frequencies when counts are too coarse: Index: predict.c === --- predict.c (revision 202133) +++ predict.c (working copy) @@ -232,8 +232,22 @@ bool probably_never_executed_bb_p (struct function *fun, const_basic_block bb) { gcc_checking_assert (fun); - if (profile_info flag_branch_probabilities) -return ((bb-count + profile_info-runs / 2) / profile_info-runs) == 0; + if (profile_status_for_function (fun) == PROFILE_READ) +{ + if ((bb-count * 4 + profile_info-runs / 2) / profile_info-runs 0) + return false; + if (!bb-frequency) + return true; + if (!ENTRY_BLOCK_PTR-frequency) + return false; + if (ENTRY_BLOCK_PTR-count ENTRY_BLOCK_PTR-count REG_BR_PROB_BASE) + { + return (RDIV (bb-frequency * ENTRY_BLOCK_PTR-count, + ENTRY_BLOCK_PTR-frequency) + REG_BR_PROB_BASE / 4); + } + return true; +} if ((!profile_info || !flag_branch_probabilities) (cgraph_get_node (fun-decl)-frequency == NODE_FREQUENCY_UNLIKELY_EXECUTED)) In other cases it was mostly loop unrolling in combination with jump threading. So I modified my script to separately report when failure happens for test trained once and test trained hundred times. FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/2422-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/2910-2.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20020413-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20030903-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20031204-1.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20031204-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20060420-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20060905-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20120427-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20120427-2.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20120808-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20121108-1.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/20121108-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/920501-6.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/920501-6.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/920726-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/981001-1.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/981001-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/990628-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/991216-2.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/991216-2.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/cmpdi-1.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/cmpdi-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/float-floor.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/float-floor.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/pr33870-1.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/pr33870-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/pr33870.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/pr33870.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/pr36093.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/pr37573.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/pr43784.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/pr43784.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/switch-1.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/switch-1.c FAIL1 /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/va-arg-22.c FAIL /home/jh/trunk/gcc/testsuite/gcc.c-torture/execute/va-arg-22.c FAIL1 is failure after one run, FIAL is failure after 100 train runs. We should take look at FAILs and see if there are bugs to fix. For FAIL1 I think it is kind of design problem: while implementing countsfrequencies the idea was that small counts do not matter, so integer arithmetic is all right. I wonder if with current C++ wonderland we can't simply switch count to a better representation. Either sreal or fixedpoint with capping (the integer overflow issues are tiring, too). Honza
[PATCH,fixincldes] AIX assert.h static_assert
In a recent change to AIX 7, the assert.h header defines static_assert. I presume this was intended not to avoid conflicts with the new C++ keyword. Sigh. However, the definition is not protected from C++, which causes problem if the header is included in C++ code, which it is in libstdc++. This fixincludes patch protects the definition so that it only applies to ISO C code. Bootstrapped on powerpc-ibm-aix7.1.0.0 Thanks, David * inclhack.def (aix_assert): New fix. * fixincl.x: Regenerate. * tests/base/assert.h [AIX_ASSERT_CHECK]: New check. Index: inclhack.def === --- inclhack.def(revision 202134) +++ inclhack.def(working copy) @@ -569,6 +569,20 @@ }; /* + * assert.h on AIX 7 redefines static_assert as _Static_assert without + * protecting C++. + */ +fix = { +hackname = aix_assert; +mach = *-*-aix*; +files = assert.h; +select= #define[ \t]static_assert[ \t]_Static_assert; +c_fix = format; +c_fix_arg = #ifndef __cplusplus\n%0\n#endif; +test_text = #define static_assert _Static_assert; +}; + +/* * complex.h on AIX 5 and AIX 6 define _Complex_I and I in terms of __I, * which only is provided by AIX xlc C99. */
Re: [PATCH,fixincldes] AIX assert.h static_assert
Sure. Looks fine. Please apply to all active branches. On Sat, Aug 31, 2013 at 4:40 PM, David Edelsohn dje@gmail.com wrote: