| Message ID | CAAe5K+W=brFLz3kZYFXTYAE+T2yJVPF=z+vp+uKh4d+9vQXJKQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: > This patch ports messages to the new dump framework, It would be great this new framework was documented somewhere. I lost track of what was agreed it would be and from the uses in the vectorizer I was never quite sure how to utilize it in other passes. I'd also like to point out two other minor things inline: [...] > 2013-08-06 Teresa Johnson <tejohnson@google.com> > Dehao Chen <dehao@google.com> > > * dumpfile.c (dump_loc): Add column number to output, make newlines > consistent. > * dumpfile.h (OPTGROUP_OTHER): Add and enable under OPTGROUP_ALL. > * ipa-inline-transform.c (clone_inlined_nodes): > (cgraph_node_opt_info): New function. > (cgraph_node_call_chain): Ditto. > (dump_inline_decision): Ditto. > (inline_call): Invoke dump_inline_decision. > * doc/invoke.texi: Document optall -fopt-info flag. > * profile.c (read_profile_edge_counts): Use new dump framework. > (compute_branch_probabilities): Ditto. > * passes.c (pass_manager::register_one_dump_file): Use OPTGROUP_OTHER > when pass not in any opt group. > * value-prof.c (check_counter): Use new dump framework. > (find_func_by_funcdef_no): Ditto. > (check_ic_target): Ditto. > * coverage.c (get_coverage_counts): Ditto. > (coverage_init): Setup new dump framework. > * ipa-inline.c (inline_small_functions): Set is_in_ipa_inline. > * ipa-inline.h (is_in_ipa_inline): Declare. > > * testsuite/gcc.dg/pr40209.c: Use -fopt-info. > * testsuite/gcc.dg/pr26570.c: Ditto. > * testsuite/gcc.dg/pr32773.c: Ditto. > * testsuite/g++.dg/tree-ssa/dom-invalid.C (struct C): Ditto. > [...] > Index: ipa-inline-transform.c > =================================================================== > --- ipa-inline-transform.c (revision 201461) > +++ ipa-inline-transform.c (working copy) > @@ -192,6 +192,108 @@ clone_inlined_nodes (struct cgraph_edge *e, bool d > } > > > +#define MAX_INT_LENGTH 20 > + > +/* Return NODE's name and profile count, if available. */ > + > +static const char * > +cgraph_node_opt_info (struct cgraph_node *node) > +{ > + char *buf; > + size_t buf_size; > + const char *bfd_name = lang_hooks.dwarf_name (node->symbol.decl, 0); > + > + if (!bfd_name) > + bfd_name = "unknown"; > + > + buf_size = strlen (bfd_name) + 1; > + if (profile_info) > + buf_size += (MAX_INT_LENGTH + 3); > + > + buf = (char *) xmalloc (buf_size); > + > + strcpy (buf, bfd_name); > + > + if (profile_info) > + sprintf (buf, "%s ("HOST_WIDEST_INT_PRINT_DEC")", buf, node->count); > + return buf; > +} I'm not sure if output of this function is aimed only at the user or if it is supposed to be used by gcc developers as well. If the latter, an incredibly useful thing is to also dump node->symbol.order too. We usually dump it after "/" sign separating it from node name. It is invaluable when examining decisions in C++ code where you can have lots of clones of a node (and also because existing dumps print it, it is easy to combine them). [...] > Index: ipa-inline.c > =================================================================== > --- ipa-inline.c (revision 201461) > +++ ipa-inline.c (working copy) > @@ -118,6 +118,9 @@ along with GCC; see the file COPYING3. If not see > static int overall_size; > static gcov_type max_count; > > +/* Global variable to denote if it is in ipa-inline pass. */ > +bool is_in_ipa_inline = false; > + > /* Return false when inlining edge E would lead to violating > limits on function unit growth or stack usage growth. > In this age of removing global variables, are you sure you need this? The only user of this seems to be a function that is only being called from inline_call... can that ever happen when not inlining? If you plan to use this function also elsewhere, perhaps the callers will know whether we are inlining or not and can provide this in a parameter? Thanks, Martin
On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: >> This patch ports messages to the new dump framework, > > It would be great this new framework was documented somewhere. I lost > track of what was agreed it would be and from the uses in the > vectorizer I was never quite sure how to utilize it in other passes. Cc'ing Sharad who implemented this - Sharad, is this documented on a wiki or elsewhere? > > I'd also like to point out two other minor things inline: > > [...] > >> 2013-08-06 Teresa Johnson <tejohnson@google.com> >> Dehao Chen <dehao@google.com> >> >> * dumpfile.c (dump_loc): Add column number to output, make newlines >> consistent. >> * dumpfile.h (OPTGROUP_OTHER): Add and enable under OPTGROUP_ALL. >> * ipa-inline-transform.c (clone_inlined_nodes): >> (cgraph_node_opt_info): New function. >> (cgraph_node_call_chain): Ditto. >> (dump_inline_decision): Ditto. >> (inline_call): Invoke dump_inline_decision. >> * doc/invoke.texi: Document optall -fopt-info flag. >> * profile.c (read_profile_edge_counts): Use new dump framework. >> (compute_branch_probabilities): Ditto. >> * passes.c (pass_manager::register_one_dump_file): Use OPTGROUP_OTHER >> when pass not in any opt group. >> * value-prof.c (check_counter): Use new dump framework. >> (find_func_by_funcdef_no): Ditto. >> (check_ic_target): Ditto. >> * coverage.c (get_coverage_counts): Ditto. >> (coverage_init): Setup new dump framework. >> * ipa-inline.c (inline_small_functions): Set is_in_ipa_inline. >> * ipa-inline.h (is_in_ipa_inline): Declare. >> >> * testsuite/gcc.dg/pr40209.c: Use -fopt-info. >> * testsuite/gcc.dg/pr26570.c: Ditto. >> * testsuite/gcc.dg/pr32773.c: Ditto. >> * testsuite/g++.dg/tree-ssa/dom-invalid.C (struct C): Ditto. >> > > [...] > >> Index: ipa-inline-transform.c >> =================================================================== >> --- ipa-inline-transform.c (revision 201461) >> +++ ipa-inline-transform.c (working copy) >> @@ -192,6 +192,108 @@ clone_inlined_nodes (struct cgraph_edge *e, bool d >> } >> >> >> +#define MAX_INT_LENGTH 20 >> + >> +/* Return NODE's name and profile count, if available. */ >> + >> +static const char * >> +cgraph_node_opt_info (struct cgraph_node *node) >> +{ >> + char *buf; >> + size_t buf_size; >> + const char *bfd_name = lang_hooks.dwarf_name (node->symbol.decl, 0); >> + >> + if (!bfd_name) >> + bfd_name = "unknown"; >> + >> + buf_size = strlen (bfd_name) + 1; >> + if (profile_info) >> + buf_size += (MAX_INT_LENGTH + 3); >> + >> + buf = (char *) xmalloc (buf_size); >> + >> + strcpy (buf, bfd_name); >> + >> + if (profile_info) >> + sprintf (buf, "%s ("HOST_WIDEST_INT_PRINT_DEC")", buf, node->count); >> + return buf; >> +} > > I'm not sure if output of this function is aimed only at the user or > if it is supposed to be used by gcc developers as well. If the > latter, an incredibly useful thing is to also dump node->symbol.order > too. We usually dump it after "/" sign separating it from node name. > It is invaluable when examining decisions in C++ code where you can > have lots of clones of a node (and also because existing dumps print > it, it is easy to combine them). The output is useful for both power users doing performance tuning of their application, and by gcc developers. Adding the id is not so useful for the former, but I agree that it is very useful for compiler developers. In fact, in the google branch version we emit more verbose information (the lipo module id and the funcdef_no) to help uniquely identify the routines and to aid in post-processing by humans and tools. So it is probably useful to add something similar here too. Is the node->symbol.order more or less unique than the funcdef_no? I see that you added a patch a few months ago to print the node->symbol.order in the function header, and it also has the advantage as you note of matching up with existing ipa dumps. > > [...] > >> Index: ipa-inline.c >> =================================================================== >> --- ipa-inline.c (revision 201461) >> +++ ipa-inline.c (working copy) >> @@ -118,6 +118,9 @@ along with GCC; see the file COPYING3. If not see >> static int overall_size; >> static gcov_type max_count; >> >> +/* Global variable to denote if it is in ipa-inline pass. */ >> +bool is_in_ipa_inline = false; >> + >> /* Return false when inlining edge E would lead to violating >> limits on function unit growth or stack usage growth. >> > > In this age of removing global variables, are you sure you need this? > The only user of this seems to be a function that is only being called > from inline_call... can that ever happen when not inlining? If you > plan to use this function also elsewhere, perhaps the callers will > know whether we are inlining or not and can provide this in a > parameter? This is to distinguish early inlining from ipa inlining. The volume of early inlining messages is too high to be on for the default setting of -fopt-info, and are not as interesting usually for performance tuning. The dumper will only emit the early inline messages under a more verbose setting (MSG_NOTE): dump_printf_loc (is_in_ipa_inline ? MSG_OPTIMIZED_LOCATIONS : MSG_NOTE ... The other way I can see to distinguish this would be to check the always_inline_functions_inlined flag on the caller's function. It could also be possible to pass down a flag from the callers of inline_call, but at least one caller (flatten_functions) is shared between early and late inlining, so the flag needs to be passed through that as well. WDYT? Thanks, Teresa > > Thanks, > > Martin
On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: >> This patch ports messages to the new dump framework, > > It would be great this new framework was documented somewhere. I lost > track of what was agreed it would be and from the uses in the > vectorizer I was never quite sure how to utilize it in other passes. Sharad, can you put the documentation in GCC wiki. In a nutshell, the new dumping interfaces produces information notes which have 'dual' outputs -- controlled by different options. When -fdump-<phase>-<pass> is on, the dump info will be dumped into the pass specific dump file, and when -fopt-info=.. is on, the information will be dumped into stderr. The dump call should be guarded by dump_enabled_p(). thanks, David > > I'd also like to point out two other minor things inline: > > [...] > >> 2013-08-06 Teresa Johnson <tejohnson@google.com> >> Dehao Chen <dehao@google.com> >> >> * dumpfile.c (dump_loc): Add column number to output, make newlines >> consistent. >> * dumpfile.h (OPTGROUP_OTHER): Add and enable under OPTGROUP_ALL. >> * ipa-inline-transform.c (clone_inlined_nodes): >> (cgraph_node_opt_info): New function. >> (cgraph_node_call_chain): Ditto. >> (dump_inline_decision): Ditto. >> (inline_call): Invoke dump_inline_decision. >> * doc/invoke.texi: Document optall -fopt-info flag. >> * profile.c (read_profile_edge_counts): Use new dump framework. >> (compute_branch_probabilities): Ditto. >> * passes.c (pass_manager::register_one_dump_file): Use OPTGROUP_OTHER >> when pass not in any opt group. >> * value-prof.c (check_counter): Use new dump framework. >> (find_func_by_funcdef_no): Ditto. >> (check_ic_target): Ditto. >> * coverage.c (get_coverage_counts): Ditto. >> (coverage_init): Setup new dump framework. >> * ipa-inline.c (inline_small_functions): Set is_in_ipa_inline. >> * ipa-inline.h (is_in_ipa_inline): Declare. >> >> * testsuite/gcc.dg/pr40209.c: Use -fopt-info. >> * testsuite/gcc.dg/pr26570.c: Ditto. >> * testsuite/gcc.dg/pr32773.c: Ditto. >> * testsuite/g++.dg/tree-ssa/dom-invalid.C (struct C): Ditto. >> > > [...] > >> Index: ipa-inline-transform.c >> =================================================================== >> --- ipa-inline-transform.c (revision 201461) >> +++ ipa-inline-transform.c (working copy) >> @@ -192,6 +192,108 @@ clone_inlined_nodes (struct cgraph_edge *e, bool d >> } >> >> >> +#define MAX_INT_LENGTH 20 >> + >> +/* Return NODE's name and profile count, if available. */ >> + >> +static const char * >> +cgraph_node_opt_info (struct cgraph_node *node) >> +{ >> + char *buf; >> + size_t buf_size; >> + const char *bfd_name = lang_hooks.dwarf_name (node->symbol.decl, 0); >> + >> + if (!bfd_name) >> + bfd_name = "unknown"; >> + >> + buf_size = strlen (bfd_name) + 1; >> + if (profile_info) >> + buf_size += (MAX_INT_LENGTH + 3); >> + >> + buf = (char *) xmalloc (buf_size); >> + >> + strcpy (buf, bfd_name); >> + >> + if (profile_info) >> + sprintf (buf, "%s ("HOST_WIDEST_INT_PRINT_DEC")", buf, node->count); >> + return buf; >> +} > > I'm not sure if output of this function is aimed only at the user or > if it is supposed to be used by gcc developers as well. If the > latter, an incredibly useful thing is to also dump node->symbol.order > too. We usually dump it after "/" sign separating it from node name. > It is invaluable when examining decisions in C++ code where you can > have lots of clones of a node (and also because existing dumps print > it, it is easy to combine them). > > [...] > >> Index: ipa-inline.c >> =================================================================== >> --- ipa-inline.c (revision 201461) >> +++ ipa-inline.c (working copy) >> @@ -118,6 +118,9 @@ along with GCC; see the file COPYING3. If not see >> static int overall_size; >> static gcov_type max_count; >> >> +/* Global variable to denote if it is in ipa-inline pass. */ >> +bool is_in_ipa_inline = false; >> + >> /* Return false when inlining edge E would lead to violating >> limits on function unit growth or stack usage growth. >> > > In this age of removing global variables, are you sure you need this? > The only user of this seems to be a function that is only being called > from inline_call... can that ever happen when not inlining? If you > plan to use this function also elsewhere, perhaps the callers will > know whether we are inlining or not and can provide this in a > parameter? > > Thanks, > > Martin
Hi, On Tue, Aug 06, 2013 at 07:14:42AM -0700, Teresa Johnson wrote: > On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: > > On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: > >> This patch ports messages to the new dump framework, > > > > It would be great this new framework was documented somewhere. I lost > > track of what was agreed it would be and from the uses in the > > vectorizer I was never quite sure how to utilize it in other passes. > > Cc'ing Sharad who implemented this - Sharad, is this documented on a > wiki or elsewhere? Thanks > > > > > I'd also like to point out two other minor things inline: > > > > [...] > > > >> 2013-08-06 Teresa Johnson <tejohnson@google.com> > >> Dehao Chen <dehao@google.com> > >> > >> * dumpfile.c (dump_loc): Add column number to output, make newlines > >> consistent. > >> * dumpfile.h (OPTGROUP_OTHER): Add and enable under OPTGROUP_ALL. > >> * ipa-inline-transform.c (clone_inlined_nodes): > >> (cgraph_node_opt_info): New function. > >> (cgraph_node_call_chain): Ditto. > >> (dump_inline_decision): Ditto. > >> (inline_call): Invoke dump_inline_decision. > >> * doc/invoke.texi: Document optall -fopt-info flag. > >> * profile.c (read_profile_edge_counts): Use new dump framework. > >> (compute_branch_probabilities): Ditto. > >> * passes.c (pass_manager::register_one_dump_file): Use OPTGROUP_OTHER > >> when pass not in any opt group. > >> * value-prof.c (check_counter): Use new dump framework. > >> (find_func_by_funcdef_no): Ditto. > >> (check_ic_target): Ditto. > >> * coverage.c (get_coverage_counts): Ditto. > >> (coverage_init): Setup new dump framework. > >> * ipa-inline.c (inline_small_functions): Set is_in_ipa_inline. > >> * ipa-inline.h (is_in_ipa_inline): Declare. > >> > >> * testsuite/gcc.dg/pr40209.c: Use -fopt-info. > >> * testsuite/gcc.dg/pr26570.c: Ditto. > >> * testsuite/gcc.dg/pr32773.c: Ditto. > >> * testsuite/g++.dg/tree-ssa/dom-invalid.C (struct C): Ditto. > >> > > > > [...] > > > >> Index: ipa-inline-transform.c > >> =================================================================== > >> --- ipa-inline-transform.c (revision 201461) > >> +++ ipa-inline-transform.c (working copy) > >> @@ -192,6 +192,108 @@ clone_inlined_nodes (struct cgraph_edge *e, bool d > >> } > >> > >> > >> +#define MAX_INT_LENGTH 20 > >> + > >> +/* Return NODE's name and profile count, if available. */ > >> + > >> +static const char * > >> +cgraph_node_opt_info (struct cgraph_node *node) > >> +{ > >> + char *buf; > >> + size_t buf_size; > >> + const char *bfd_name = lang_hooks.dwarf_name (node->symbol.decl, 0); > >> + > >> + if (!bfd_name) > >> + bfd_name = "unknown"; > >> + > >> + buf_size = strlen (bfd_name) + 1; > >> + if (profile_info) > >> + buf_size += (MAX_INT_LENGTH + 3); > >> + > >> + buf = (char *) xmalloc (buf_size); > >> + > >> + strcpy (buf, bfd_name); > >> + > >> + if (profile_info) > >> + sprintf (buf, "%s ("HOST_WIDEST_INT_PRINT_DEC")", buf, node->count); > >> + return buf; > >> +} > > > > I'm not sure if output of this function is aimed only at the user or > > if it is supposed to be used by gcc developers as well. If the > > latter, an incredibly useful thing is to also dump node->symbol.order > > too. We usually dump it after "/" sign separating it from node name. > > It is invaluable when examining decisions in C++ code where you can > > have lots of clones of a node (and also because existing dumps print > > it, it is easy to combine them). > > The output is useful for both power users doing performance tuning of > their application, and by gcc developers. Adding the id is not so > useful for the former, but I agree that it is very useful for compiler > developers. In fact, in the google branch version we emit more verbose > information (the lipo module id and the funcdef_no) to help uniquely > identify the routines and to aid in post-processing by humans and > tools. So it is probably useful to add something similar here too. Is > the node->symbol.order more or less unique than the funcdef_no? I see > that you added a patch a few months ago to print the > node->symbol.order in the function header, and it also has the > advantage as you note of matching up with existing ipa dumps. node->symbol.order is unique and if I remember correctly, it is not even recycled. Clones, inline clones, thunks, every symbol table node gets its own symbol order so it should be more unique than funcdef_no. On the other hand it may be a bit cryptic for users but at the same time it is only one number. > > > > > [...] > > > >> Index: ipa-inline.c > >> =================================================================== > >> --- ipa-inline.c (revision 201461) > >> +++ ipa-inline.c (working copy) > >> @@ -118,6 +118,9 @@ along with GCC; see the file COPYING3. If not see > >> static int overall_size; > >> static gcov_type max_count; > >> > >> +/* Global variable to denote if it is in ipa-inline pass. */ > >> +bool is_in_ipa_inline = false; > >> + > >> /* Return false when inlining edge E would lead to violating > >> limits on function unit growth or stack usage growth. > >> > > > > In this age of removing global variables, are you sure you need this? > > The only user of this seems to be a function that is only being called > > from inline_call... can that ever happen when not inlining? If you > > plan to use this function also elsewhere, perhaps the callers will > > know whether we are inlining or not and can provide this in a > > parameter? > > This is to distinguish early inlining from ipa inlining. Oh, right, I did not realize that the IPA part was the important bit of the name. > The volume of > early inlining messages is too high to be on for the default setting > of -fopt-info, and are not as interesting usually for performance > tuning. The dumper will only emit the early inline messages under a > more verbose setting (MSG_NOTE): > dump_printf_loc (is_in_ipa_inline ? MSG_OPTIMIZED_LOCATIONS : MSG_NOTE ... > The other way I can see to distinguish this would be to check the > always_inline_functions_inlined flag on the caller's function. It > could also be possible to pass down a flag from the callers of > inline_call, but at least one caller (flatten_functions) is shared > between early and late inlining, so the flag needs to be passed > through that as well. WDYT? Did you mean flatten_function? It already has a bool "early" parameter. But I can see that being able to quickly figure out whether we are in early inliner or ipa inliner without much hassle is useful enough to justify a global variable a month ago, however I suppose we should not be introducing them now and so you'd have to put such stuff into... well, you'd probably have to put into the universe object somewhere because it is basically shared between two passes. Another option, even though somewhat hackish, would be to look at current_pass and see which pass it is. I don't know, do what is easier or what you like more, just be aware of the problem. Thanks, Martin
On Tue, Aug 6, 2013 at 8:57 AM, Xinliang David Li <davidxl@google.com> wrote: > On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: >> Hi, >> >> On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: >>> This patch ports messages to the new dump framework, >> >> It would be great this new framework was documented somewhere. I lost >> track of what was agreed it would be and from the uses in the >> vectorizer I was never quite sure how to utilize it in other passes. > > Sharad, can you put the documentation in GCC wiki. Sure. I had user documentation in form of gcc info. But I will add more developer details to a GCC wiki. Thanks, Sharad > In a nutshell, the new dumping interfaces produces information notes > which have 'dual' outputs -- controlled by different options. When > -fdump-<phase>-<pass> is on, the dump info will be dumped into the > pass specific dump file, and when -fopt-info=.. is on, the information > will be dumped into stderr. > > The dump call should be guarded by dump_enabled_p(). > > thanks, > > David > >> >> I'd also like to point out two other minor things inline: >> >> [...] >> >>> 2013-08-06 Teresa Johnson <tejohnson@google.com> >>> Dehao Chen <dehao@google.com> >>> >>> * dumpfile.c (dump_loc): Add column number to output, make newlines >>> consistent. >>> * dumpfile.h (OPTGROUP_OTHER): Add and enable under OPTGROUP_ALL. >>> * ipa-inline-transform.c (clone_inlined_nodes): >>> (cgraph_node_opt_info): New function. >>> (cgraph_node_call_chain): Ditto. >>> (dump_inline_decision): Ditto. >>> (inline_call): Invoke dump_inline_decision. >>> * doc/invoke.texi: Document optall -fopt-info flag. >>> * profile.c (read_profile_edge_counts): Use new dump framework. >>> (compute_branch_probabilities): Ditto. >>> * passes.c (pass_manager::register_one_dump_file): Use OPTGROUP_OTHER >>> when pass not in any opt group. >>> * value-prof.c (check_counter): Use new dump framework. >>> (find_func_by_funcdef_no): Ditto. >>> (check_ic_target): Ditto. >>> * coverage.c (get_coverage_counts): Ditto. >>> (coverage_init): Setup new dump framework. >>> * ipa-inline.c (inline_small_functions): Set is_in_ipa_inline. >>> * ipa-inline.h (is_in_ipa_inline): Declare. >>> >>> * testsuite/gcc.dg/pr40209.c: Use -fopt-info. >>> * testsuite/gcc.dg/pr26570.c: Ditto. >>> * testsuite/gcc.dg/pr32773.c: Ditto. >>> * testsuite/g++.dg/tree-ssa/dom-invalid.C (struct C): Ditto. >>> >> >> [...] >> >>> Index: ipa-inline-transform.c >>> =================================================================== >>> --- ipa-inline-transform.c (revision 201461) >>> +++ ipa-inline-transform.c (working copy) >>> @@ -192,6 +192,108 @@ clone_inlined_nodes (struct cgraph_edge *e, bool d >>> } >>> >>> >>> +#define MAX_INT_LENGTH 20 >>> + >>> +/* Return NODE's name and profile count, if available. */ >>> + >>> +static const char * >>> +cgraph_node_opt_info (struct cgraph_node *node) >>> +{ >>> + char *buf; >>> + size_t buf_size; >>> + const char *bfd_name = lang_hooks.dwarf_name (node->symbol.decl, 0); >>> + >>> + if (!bfd_name) >>> + bfd_name = "unknown"; >>> + >>> + buf_size = strlen (bfd_name) + 1; >>> + if (profile_info) >>> + buf_size += (MAX_INT_LENGTH + 3); >>> + >>> + buf = (char *) xmalloc (buf_size); >>> + >>> + strcpy (buf, bfd_name); >>> + >>> + if (profile_info) >>> + sprintf (buf, "%s ("HOST_WIDEST_INT_PRINT_DEC")", buf, node->count); >>> + return buf; >>> +} >> >> I'm not sure if output of this function is aimed only at the user or >> if it is supposed to be used by gcc developers as well. If the >> latter, an incredibly useful thing is to also dump node->symbol.order >> too. We usually dump it after "/" sign separating it from node name. >> It is invaluable when examining decisions in C++ code where you can >> have lots of clones of a node (and also because existing dumps print >> it, it is easy to combine them). >> >> [...] >> >>> Index: ipa-inline.c >>> =================================================================== >>> --- ipa-inline.c (revision 201461) >>> +++ ipa-inline.c (working copy) >>> @@ -118,6 +118,9 @@ along with GCC; see the file COPYING3. If not see >>> static int overall_size; >>> static gcov_type max_count; >>> >>> +/* Global variable to denote if it is in ipa-inline pass. */ >>> +bool is_in_ipa_inline = false; >>> + >>> /* Return false when inlining edge E would lead to violating >>> limits on function unit growth or stack usage growth. >>> >> >> In this age of removing global variables, are you sure you need this? >> The only user of this seems to be a function that is only being called >> from inline_call... can that ever happen when not inlining? If you >> plan to use this function also elsewhere, perhaps the callers will >> know whether we are inlining or not and can provide this in a >> parameter? >> >> Thanks, >> >> Martin
On Tue, Aug 6, 2013 at 9:01 AM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > On Tue, Aug 06, 2013 at 07:14:42AM -0700, Teresa Johnson wrote: >> On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: >> > On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: >> >> This patch ports messages to the new dump framework, >> > >> > It would be great this new framework was documented somewhere. I lost >> > track of what was agreed it would be and from the uses in the >> > vectorizer I was never quite sure how to utilize it in other passes. >> >> Cc'ing Sharad who implemented this - Sharad, is this documented on a >> wiki or elsewhere? > > Thanks > >> >> > >> > I'd also like to point out two other minor things inline: >> > >> > [...] >> > >> >> 2013-08-06 Teresa Johnson <tejohnson@google.com> >> >> Dehao Chen <dehao@google.com> >> >> >> >> * dumpfile.c (dump_loc): Add column number to output, make newlines >> >> consistent. >> >> * dumpfile.h (OPTGROUP_OTHER): Add and enable under OPTGROUP_ALL. >> >> * ipa-inline-transform.c (clone_inlined_nodes): >> >> (cgraph_node_opt_info): New function. >> >> (cgraph_node_call_chain): Ditto. >> >> (dump_inline_decision): Ditto. >> >> (inline_call): Invoke dump_inline_decision. >> >> * doc/invoke.texi: Document optall -fopt-info flag. >> >> * profile.c (read_profile_edge_counts): Use new dump framework. >> >> (compute_branch_probabilities): Ditto. >> >> * passes.c (pass_manager::register_one_dump_file): Use OPTGROUP_OTHER >> >> when pass not in any opt group. >> >> * value-prof.c (check_counter): Use new dump framework. >> >> (find_func_by_funcdef_no): Ditto. >> >> (check_ic_target): Ditto. >> >> * coverage.c (get_coverage_counts): Ditto. >> >> (coverage_init): Setup new dump framework. >> >> * ipa-inline.c (inline_small_functions): Set is_in_ipa_inline. >> >> * ipa-inline.h (is_in_ipa_inline): Declare. >> >> >> >> * testsuite/gcc.dg/pr40209.c: Use -fopt-info. >> >> * testsuite/gcc.dg/pr26570.c: Ditto. >> >> * testsuite/gcc.dg/pr32773.c: Ditto. >> >> * testsuite/g++.dg/tree-ssa/dom-invalid.C (struct C): Ditto. >> >> >> > >> > [...] >> > >> >> Index: ipa-inline-transform.c >> >> =================================================================== >> >> --- ipa-inline-transform.c (revision 201461) >> >> +++ ipa-inline-transform.c (working copy) >> >> @@ -192,6 +192,108 @@ clone_inlined_nodes (struct cgraph_edge *e, bool d >> >> } >> >> >> >> >> >> +#define MAX_INT_LENGTH 20 >> >> + >> >> +/* Return NODE's name and profile count, if available. */ >> >> + >> >> +static const char * >> >> +cgraph_node_opt_info (struct cgraph_node *node) >> >> +{ >> >> + char *buf; >> >> + size_t buf_size; >> >> + const char *bfd_name = lang_hooks.dwarf_name (node->symbol.decl, 0); >> >> + >> >> + if (!bfd_name) >> >> + bfd_name = "unknown"; >> >> + >> >> + buf_size = strlen (bfd_name) + 1; >> >> + if (profile_info) >> >> + buf_size += (MAX_INT_LENGTH + 3); >> >> + >> >> + buf = (char *) xmalloc (buf_size); >> >> + >> >> + strcpy (buf, bfd_name); >> >> + >> >> + if (profile_info) >> >> + sprintf (buf, "%s ("HOST_WIDEST_INT_PRINT_DEC")", buf, node->count); >> >> + return buf; >> >> +} >> > >> > I'm not sure if output of this function is aimed only at the user or >> > if it is supposed to be used by gcc developers as well. If the >> > latter, an incredibly useful thing is to also dump node->symbol.order >> > too. We usually dump it after "/" sign separating it from node name. >> > It is invaluable when examining decisions in C++ code where you can >> > have lots of clones of a node (and also because existing dumps print >> > it, it is easy to combine them). >> >> The output is useful for both power users doing performance tuning of >> their application, and by gcc developers. Adding the id is not so >> useful for the former, but I agree that it is very useful for compiler >> developers. In fact, in the google branch version we emit more verbose >> information (the lipo module id and the funcdef_no) to help uniquely >> identify the routines and to aid in post-processing by humans and >> tools. So it is probably useful to add something similar here too. Is >> the node->symbol.order more or less unique than the funcdef_no? I see >> that you added a patch a few months ago to print the >> node->symbol.order in the function header, and it also has the >> advantage as you note of matching up with existing ipa dumps. > > node->symbol.order is unique and if I remember correctly, it is not > even recycled. Clones, inline clones, thunks, every symbol table node > gets its own symbol order so it should be more unique than funcdef_no. > On the other hand it may be a bit cryptic for users but at the same > time it is only one number. Ok, I am going to go ahead and add this to the output. > >> >> > >> > [...] >> > >> >> Index: ipa-inline.c >> >> =================================================================== >> >> --- ipa-inline.c (revision 201461) >> >> +++ ipa-inline.c (working copy) >> >> @@ -118,6 +118,9 @@ along with GCC; see the file COPYING3. If not see >> >> static int overall_size; >> >> static gcov_type max_count; >> >> >> >> +/* Global variable to denote if it is in ipa-inline pass. */ >> >> +bool is_in_ipa_inline = false; >> >> + >> >> /* Return false when inlining edge E would lead to violating >> >> limits on function unit growth or stack usage growth. >> >> >> > >> > In this age of removing global variables, are you sure you need this? >> > The only user of this seems to be a function that is only being called >> > from inline_call... can that ever happen when not inlining? If you >> > plan to use this function also elsewhere, perhaps the callers will >> > know whether we are inlining or not and can provide this in a >> > parameter? >> >> This is to distinguish early inlining from ipa inlining. > > Oh, right, I did not realize that the IPA part was the important bit > of the name. > >> The volume of >> early inlining messages is too high to be on for the default setting >> of -fopt-info, and are not as interesting usually for performance >> tuning. The dumper will only emit the early inline messages under a >> more verbose setting (MSG_NOTE): >> dump_printf_loc (is_in_ipa_inline ? MSG_OPTIMIZED_LOCATIONS : MSG_NOTE ... >> The other way I can see to distinguish this would be to check the >> always_inline_functions_inlined flag on the caller's function. It >> could also be possible to pass down a flag from the callers of >> inline_call, but at least one caller (flatten_functions) is shared >> between early and late inlining, so the flag needs to be passed >> through that as well. WDYT? > > Did you mean flatten_function? It already has a bool "early" > parameter. But I can see that being able to quickly figure out > whether we are in early inliner or ipa inliner without much hassle is > useful enough to justify a global variable a month ago, however I > suppose we should not be introducing them now and so you'd have to put > such stuff into... well, you'd probably have to put into the universe > object somewhere because it is basically shared between two passes. > Another option, even though somewhat hackish, would be to look at > current_pass and see which pass it is. I don't know, do what is > easier or what you like more, just be aware of the problem. After thinking about this some more, I think passing down an early flag from callers is the cleanest way to go. I'll fix these and post a new patch later today. Thanks, Teresa > > Thanks, > > Martin
On Tue, Aug 06, 2013 at 09:22:02AM -0700, Sharad Singhai wrote: > On Tue, Aug 6, 2013 at 8:57 AM, Xinliang David Li <davidxl@google.com> wrote: > > On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: > >> Hi, > >> > >> On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: > >>> This patch ports messages to the new dump framework, > >> > >> It would be great this new framework was documented somewhere. I lost > >> track of what was agreed it would be and from the uses in the > >> vectorizer I was never quite sure how to utilize it in other passes. > > > > Sharad, can you put the documentation in GCC wiki. > > Sure. I had user documentation in form of gcc info. But I will add > more developer details to a GCC wiki. > I have built trunk gccint.info yesterday but could not find any string dump_enabled_p there, for example. And when I quickly searched just for the string "dump," I did not find any thing that looked like dumping infrastructure either. OTOH, I agree that fie would be the best place for the documentation. Or did I just miss it? What section is it in then? Thanks, Martin
On Tue, Aug 6, 2013 at 10:10 AM, Martin Jambor <mjambor@suse.cz> wrote: > On Tue, Aug 06, 2013 at 09:22:02AM -0700, Sharad Singhai wrote: >> On Tue, Aug 6, 2013 at 8:57 AM, Xinliang David Li <davidxl@google.com> wrote: >> > On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: >> >> Hi, >> >> >> >> On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: >> >>> This patch ports messages to the new dump framework, >> >> >> >> It would be great this new framework was documented somewhere. I lost >> >> track of what was agreed it would be and from the uses in the >> >> vectorizer I was never quite sure how to utilize it in other passes. >> > >> > Sharad, can you put the documentation in GCC wiki. >> >> Sure. I had user documentation in form of gcc info. But I will add >> more developer details to a GCC wiki. >> > > I have built trunk gccint.info yesterday but could not find any string > dump_enabled_p there, for example. And when I quickly searched just > for the string "dump," I did not find any thing that looked like > dumping infrastructure either. OTOH, I agree that fie would be the > best place for the documentation. > > Or did I just miss it? What section is it in then? Actually, the user-facing documentation is in doc/invoke.texi. However, that doesn't describe dump_enabled_p. Do you think gccint.info would be a good place? I can add documentation there instead of creating a GCC wiki. Thanks, Sharad > Thanks, > > Martin >
yes -- if this is the place developers look at the most. David On Tue, Aug 6, 2013 at 10:18 AM, Sharad Singhai <singhai@google.com> wrote: > On Tue, Aug 6, 2013 at 10:10 AM, Martin Jambor <mjambor@suse.cz> wrote: >> On Tue, Aug 06, 2013 at 09:22:02AM -0700, Sharad Singhai wrote: >>> On Tue, Aug 6, 2013 at 8:57 AM, Xinliang David Li <davidxl@google.com> wrote: >>> > On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: >>> >> Hi, >>> >> >>> >> On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: >>> >>> This patch ports messages to the new dump framework, >>> >> >>> >> It would be great this new framework was documented somewhere. I lost >>> >> track of what was agreed it would be and from the uses in the >>> >> vectorizer I was never quite sure how to utilize it in other passes. >>> > >>> > Sharad, can you put the documentation in GCC wiki. >>> >>> Sure. I had user documentation in form of gcc info. But I will add >>> more developer details to a GCC wiki. >>> >> >> I have built trunk gccint.info yesterday but could not find any string >> dump_enabled_p there, for example. And when I quickly searched just >> for the string "dump," I did not find any thing that looked like >> dumping infrastructure either. OTOH, I agree that fie would be the >> best place for the documentation. >> >> Or did I just miss it? What section is it in then? > > Actually, the user-facing documentation is in doc/invoke.texi. > However, that doesn't describe dump_enabled_p. Do you think > gccint.info would be a good place? I can add documentation there > instead of creating a GCC wiki. > > Thanks, > Sharad > >> Thanks, >> >> Martin >>
Hi, On Tue, Aug 06, 2013 at 10:18:05AM -0700, Sharad Singhai wrote: > On Tue, Aug 6, 2013 at 10:10 AM, Martin Jambor <mjambor@suse.cz> wrote: > > On Tue, Aug 06, 2013 at 09:22:02AM -0700, Sharad Singhai wrote: > >> On Tue, Aug 6, 2013 at 8:57 AM, Xinliang David Li <davidxl@google.com> wrote: > >> > On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: > >> >> Hi, > >> >> > >> >> On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: > >> >>> This patch ports messages to the new dump framework, > >> >> > >> >> It would be great this new framework was documented somewhere. I lost > >> >> track of what was agreed it would be and from the uses in the > >> >> vectorizer I was never quite sure how to utilize it in other passes. > >> > > >> > Sharad, can you put the documentation in GCC wiki. > >> > >> Sure. I had user documentation in form of gcc info. But I will add > >> more developer details to a GCC wiki. > >> > > > > I have built trunk gccint.info yesterday but could not find any string > > dump_enabled_p there, for example. And when I quickly searched just > > for the string "dump," I did not find any thing that looked like > > dumping infrastructure either. OTOH, I agree that fie would be the > > best place for the documentation. > > > > Or did I just miss it? What section is it in then? > > Actually, the user-facing documentation is in doc/invoke.texi. > However, that doesn't describe dump_enabled_p. Do you think > gccint.info would be a good place? I can add documentation there > instead of creating a GCC wiki. > please do not forget about this, otherwise few people will use your framework. Thanks, Martin
Committed documentation as r206161. Sorry about the delay. Thanks, Sharad On Thu, Nov 28, 2013 at 10:03 AM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > On Tue, Aug 06, 2013 at 10:18:05AM -0700, Sharad Singhai wrote: >> On Tue, Aug 6, 2013 at 10:10 AM, Martin Jambor <mjambor@suse.cz> wrote: >> > On Tue, Aug 06, 2013 at 09:22:02AM -0700, Sharad Singhai wrote: >> >> On Tue, Aug 6, 2013 at 8:57 AM, Xinliang David Li <davidxl@google.com> wrote: >> >> > On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: >> >> >> Hi, >> >> >> >> >> >> On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: >> >> >>> This patch ports messages to the new dump framework, >> >> >> >> >> >> It would be great this new framework was documented somewhere. I lost >> >> >> track of what was agreed it would be and from the uses in the >> >> >> vectorizer I was never quite sure how to utilize it in other passes. >> >> > >> >> > Sharad, can you put the documentation in GCC wiki. >> >> >> >> Sure. I had user documentation in form of gcc info. But I will add >> >> more developer details to a GCC wiki. >> >> >> > >> > I have built trunk gccint.info yesterday but could not find any string >> > dump_enabled_p there, for example. And when I quickly searched just >> > for the string "dump," I did not find any thing that looked like >> > dumping infrastructure either. OTOH, I agree that fie would be the >> > best place for the documentation. >> > >> > Or did I just miss it? What section is it in then? >> >> Actually, the user-facing documentation is in doc/invoke.texi. >> However, that doesn't describe dump_enabled_p. Do you think >> gccint.info would be a good place? I can add documentation there >> instead of creating a GCC wiki. >> > > please do not forget about this, otherwise few people will use your > framework. > > Thanks, > > Martin
thanks. David On Fri, Dec 20, 2013 at 11:48 PM, Sharad Singhai <singhai@google.com> wrote: > Committed documentation as r206161. Sorry about the delay. > > Thanks, > Sharad > > On Thu, Nov 28, 2013 at 10:03 AM, Martin Jambor <mjambor@suse.cz> wrote: >> Hi, >> >> On Tue, Aug 06, 2013 at 10:18:05AM -0700, Sharad Singhai wrote: >>> On Tue, Aug 6, 2013 at 10:10 AM, Martin Jambor <mjambor@suse.cz> wrote: >>> > On Tue, Aug 06, 2013 at 09:22:02AM -0700, Sharad Singhai wrote: >>> >> On Tue, Aug 6, 2013 at 8:57 AM, Xinliang David Li <davidxl@google.com> wrote: >>> >> > On Tue, Aug 6, 2013 at 5:37 AM, Martin Jambor <mjambor@suse.cz> wrote: >>> >> >> Hi, >>> >> >> >>> >> >> On Mon, Aug 05, 2013 at 10:37:00PM -0700, Teresa Johnson wrote: >>> >> >>> This patch ports messages to the new dump framework, >>> >> >> >>> >> >> It would be great this new framework was documented somewhere. I lost >>> >> >> track of what was agreed it would be and from the uses in the >>> >> >> vectorizer I was never quite sure how to utilize it in other passes. >>> >> > >>> >> > Sharad, can you put the documentation in GCC wiki. >>> >> >>> >> Sure. I had user documentation in form of gcc info. But I will add >>> >> more developer details to a GCC wiki. >>> >> >>> > >>> > I have built trunk gccint.info yesterday but could not find any string >>> > dump_enabled_p there, for example. And when I quickly searched just >>> > for the string "dump," I did not find any thing that looked like >>> > dumping infrastructure either. OTOH, I agree that fie would be the >>> > best place for the documentation. >>> > >>> > Or did I just miss it? What section is it in then? >>> >>> Actually, the user-facing documentation is in doc/invoke.texi. >>> However, that doesn't describe dump_enabled_p. Do you think >>> gccint.info would be a good place? I can add documentation there >>> instead of creating a GCC wiki. >>> >> >> please do not forget about this, otherwise few people will use your >> framework. >> >> Thanks, >> >> Martin
Index: dumpfile.c =================================================================== --- dumpfile.c (revision 201461) +++ dumpfile.c (working copy) @@ -257,16 +257,18 @@ dump_open_alternate_stream (struct dump_file_info void dump_loc (int dump_kind, FILE *dfile, source_location loc) { - /* Currently vectorization passes print location information. */ if (dump_kind) { + /* Ensure dump message starts on a new line. */ + fprintf (dfile, "\n"); if (LOCATION_LOCUS (loc) > BUILTINS_LOCATION) - fprintf (dfile, "\n%s:%d: note: ", LOCATION_FILE (loc), - LOCATION_LINE (loc)); + fprintf (dfile, "%s:%d:%d: note: ", LOCATION_FILE (loc), + LOCATION_LINE (loc), LOCATION_COLUMN (loc)); else if (current_function_decl) - fprintf (dfile, "\n%s:%d: note: ", + fprintf (dfile, "%s:%d:%d: note: ", DECL_SOURCE_FILE (current_function_decl), - DECL_SOURCE_LINE (current_function_decl)); + DECL_SOURCE_LINE (current_function_decl), + DECL_SOURCE_COLUMN (current_function_decl)); } } Index: dumpfile.h =================================================================== --- dumpfile.h (revision 201461) +++ dumpfile.h (working copy) @@ -97,8 +97,9 @@ enum tree_dump_index #define OPTGROUP_LOOP (1 << 2) /* Loop optimization passes */ #define OPTGROUP_INLINE (1 << 3) /* Inlining passes */ #define OPTGROUP_VEC (1 << 4) /* Vectorization passes */ +#define OPTGROUP_OTHER (1 << 5) /* All other passes */ #define OPTGROUP_ALL (OPTGROUP_IPA | OPTGROUP_LOOP | OPTGROUP_INLINE \ - | OPTGROUP_VEC) + | OPTGROUP_VEC | OPTGROUP_OTHER) /* Define a tree dump switch. */ struct dump_file_info Index: ipa-inline-transform.c =================================================================== --- ipa-inline-transform.c (revision 201461) +++ ipa-inline-transform.c (working copy) @@ -192,6 +192,108 @@ clone_inlined_nodes (struct cgraph_edge *e, bool d } +#define MAX_INT_LENGTH 20 + +/* Return NODE's name and profile count, if available. */ + +static const char * +cgraph_node_opt_info (struct cgraph_node *node) +{ + char *buf; + size_t buf_size; + const char *bfd_name = lang_hooks.dwarf_name (node->symbol.decl, 0); + + if (!bfd_name) + bfd_name = "unknown"; + + buf_size = strlen (bfd_name) + 1; + if (profile_info) + buf_size += (MAX_INT_LENGTH + 3); + + buf = (char *) xmalloc (buf_size); + + strcpy (buf, bfd_name); + + if (profile_info) + sprintf (buf, "%s ("HOST_WIDEST_INT_PRINT_DEC")", buf, node->count); + return buf; +} + + +/* Return CALLER's inlined call chain. Save the cgraph_node of the ultimate + function that the caller is inlined to in FINAL_CALLER. */ + +static const char * +cgraph_node_call_chain (struct cgraph_node *caller, + struct cgraph_node **final_caller) +{ + struct cgraph_node *node; + const char *via_str = " (via inline instance"; + size_t current_string_len = strlen (via_str) + 1; + size_t buf_size = current_string_len; + char *buf = (char *) xmalloc (buf_size); + + buf[0] = 0; + gcc_assert (caller->global.inlined_to != NULL); + strcat (buf, via_str); + for (node = caller; node->global.inlined_to != NULL; + node = node->callers->caller) + { + const char *name = cgraph_node_opt_info (node); + current_string_len += (strlen (name) + 1); + if (current_string_len >= buf_size) + { + buf_size = current_string_len * 2; + buf = (char *) xrealloc (buf, buf_size); + } + strcat (buf, " "); + strcat (buf, name); + } + strcat (buf, ")"); + *final_caller = node; + return buf; +} + + +/* Dump the inline decision of EDGE. */ + +static void +dump_inline_decision (struct cgraph_edge *edge) +{ + location_t locus; + const char *inline_chain_text; + const char *call_count_text; + struct cgraph_node *final_caller = edge->caller; + + if (final_caller->global.inlined_to != NULL) + inline_chain_text = cgraph_node_call_chain (final_caller, &final_caller); + else + inline_chain_text = ""; + + if (edge->count > 0) + { + const char *call_count_str = " with call count "; + char *buf = (char *) xmalloc (strlen (call_count_str) + MAX_INT_LENGTH); + sprintf (buf, "%s"HOST_WIDEST_INT_PRINT_DEC, call_count_str, + edge->count); + call_count_text = buf; + } + else + { + call_count_text = ""; + } + + locus = gimple_location (edge->call_stmt); + dump_printf_loc (is_in_ipa_inline ? MSG_OPTIMIZED_LOCATIONS : MSG_NOTE, + locus, + "%s inlined into %s%s%s\n", + cgraph_node_opt_info (edge->callee), + cgraph_node_opt_info (final_caller), + call_count_text, + inline_chain_text); +} + + /* Mark edge E as inlined and update callgraph accordingly. UPDATE_ORIGINAL specify whether profile of original function should be updated. If any new indirect edges are discovered in the process, add them to NEW_EDGES, unless @@ -218,6 +320,9 @@ inline_call (struct cgraph_edge *e, bool update_or bool predicated = inline_edge_summary (e)->predicate != NULL; #endif + if (dump_enabled_p ()) + dump_inline_decision (e); + /* Don't inline inlined edges. */ gcc_assert (e->inline_failed); /* Don't even think of inlining inline clone. */ Index: doc/invoke.texi =================================================================== --- doc/invoke.texi (revision 201461) +++ doc/invoke.texi (working copy) @@ -6234,6 +6234,9 @@ Enable dumps from all loop optimizations. Enable dumps from all inlining optimizations. @item vec Enable dumps from all vectorization optimizations. +@item optall +Enable dumps from all optimizations. This is a superset of +the optimization groups listed above. @end table For example, Index: profile.c =================================================================== --- profile.c (revision 201461) +++ profile.c (working copy) @@ -432,8 +432,8 @@ read_profile_edge_counts (gcov_type *exec_counts) if (flag_profile_correction) { static bool informed = 0; - if (!informed) - inform (input_location, + if (dump_enabled_p () && !informed) + dump_printf_loc (MSG_NOTE, input_location, "corrupted profile info: edge count exceeds maximal count"); informed = 1; } @@ -692,10 +692,11 @@ compute_branch_probabilities (unsigned cfg_checksu { /* Inconsistency detected. Make it flow-consistent. */ static int informed = 0; - if (informed == 0) + if (dump_enabled_p () && informed == 0) { informed = 1; - inform (input_location, "correcting inconsistent profile data"); + dump_printf_loc (MSG_NOTE, input_location, + "correcting inconsistent profile data"); } correct_negative_edge_counts (); /* Set bb counts to the sum of the outgoing edge counts */ Index: passes.c =================================================================== --- passes.c (revision 201461) +++ passes.c (working copy) @@ -524,6 +524,11 @@ pass_manager::register_one_dump_file (struct opt_p flag_name = concat (prefix, name, num, NULL); glob_name = concat (prefix, name, NULL); optgroup_flags |= pass->optinfo_flags; + /* For any passes that do not have an optgroup set, and which are not + IPA passes setup above, set the optgroup to OPTGROUP_OTHER so that + any dump messages are emitted properly under -fopt-info(-optall). */ + if (optgroup_flags == OPTGROUP_NONE) + optgroup_flags = OPTGROUP_OTHER; id = dump_register (dot_name, flag_name, glob_name, flags, optgroup_flags); set_pass_for_id (id, pass); full_name = concat (prefix, pass->name, num, NULL); Index: value-prof.c =================================================================== --- value-prof.c (revision 201461) +++ value-prof.c (working copy) @@ -585,9 +585,11 @@ check_counter (gimple stmt, const char * name, : DECL_SOURCE_LOCATION (current_function_decl); if (flag_profile_correction) { - inform (locus, "correcting inconsistent value profile: " - "%s profiler overall count (%d) does not match BB count " - "(%d)", name, (int)*all, (int)bb_count); + if (dump_enabled_p ()) + dump_printf_loc (MSG_MISSED_OPTIMIZATION, locus, + "correcting inconsistent value profile: %s " + "profiler overall count (%d) does not match BB " + "count (%d)", name, (int)*all, (int)bb_count); *all = bb_count; if (*count > *all) *count = *all; @@ -1209,9 +1211,11 @@ find_func_by_funcdef_no (int func_id) int max_id = get_last_funcdef_no (); if (func_id >= max_id || cgraph_node_map[func_id] == NULL) { - if (flag_profile_correction) - inform (DECL_SOURCE_LOCATION (current_function_decl), - "Inconsistent profile: indirect call target (%d) does not exist", func_id); + if (flag_profile_correction && dump_enabled_p ()) + dump_printf_loc (MSG_MISSED_OPTIMIZATION, + DECL_SOURCE_LOCATION (current_function_decl), + "Inconsistent profile: indirect call target (%d) " + "does not exist", func_id); else error ("Inconsistent profile: indirect call target (%d) does not exist", func_id); @@ -1235,8 +1239,10 @@ check_ic_target (gimple call_stmt, struct cgraph_n return true; locus = gimple_location (call_stmt); - inform (locus, "Skipping target %s with mismatching types for icall ", - cgraph_node_name (target)); + if (dump_enabled_p ()) + dump_printf_loc (MSG_MISSED_OPTIMIZATION, locus, + "Skipping target %s with mismatching types for icall ", + cgraph_node_name (target)); return false; } Index: coverage.c =================================================================== --- coverage.c (revision 201461) +++ coverage.c (working copy) @@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. If not see #include "langhooks.h" #include "hash-table.h" #include "tree-iterator.h" +#include "tree-pass.h" #include "cgraph.h" #include "dumpfile.h" #include "diagnostic-core.h" @@ -341,11 +342,13 @@ get_coverage_counts (unsigned counter, unsigned ex { static int warned = 0; - if (!warned++) - inform (input_location, (flag_guess_branch_prob - ? "file %s not found, execution counts estimated" - : "file %s not found, execution counts assumed to be zero"), - da_file_name); + if (!warned++ && dump_enabled_p ()) + dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, input_location, + (flag_guess_branch_prob + ? "file %s not found, execution counts estimated" + : "file %s not found, execution counts assumed to " + "be zero"), + da_file_name); return NULL; } @@ -369,21 +372,25 @@ get_coverage_counts (unsigned counter, unsigned ex warning_at (input_location, OPT_Wcoverage_mismatch, "the control flow of function %qE does not match " "its profile data (counter %qs)", id, ctr_names[counter]); - if (warning_printed) + if (warning_printed && dump_enabled_p ()) { - inform (input_location, "use -Wno-error=coverage-mismatch to tolerate " - "the mismatch but performance may drop if the function is hot"); + dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, input_location, + "use -Wno-error=coverage-mismatch to tolerate " + "the mismatch but performance may drop if the " + "function is hot"); if (!seen_error () && !warned++) { - inform (input_location, "coverage mismatch ignored"); - inform (input_location, flag_guess_branch_prob - ? G_("execution counts estimated") - : G_("execution counts assumed to be zero")); + dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, input_location, + "coverage mismatch ignored"); + dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, input_location, + flag_guess_branch_prob + ? G_("execution counts estimated") + : G_("execution counts assumed to be zero")); if (!flag_guess_branch_prob) - inform (input_location, - "this can result in poorly optimized code"); + dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, input_location, + "this can result in poorly optimized code"); } } @@ -1103,6 +1110,11 @@ coverage_init (const char *filename) int len = strlen (filename); int prefix_len = 0; + /* Since coverage_init is invoked very early, before the pass + manager, we need to set up the dumping explicitly. This is + similar to the handling in finish_optimization_passes. */ + dump_start (pass_profile.pass.static_pass_number, NULL); + if (!profile_data_prefix && !IS_ABSOLUTE_PATH (filename)) profile_data_prefix = getpwd (); @@ -1145,6 +1157,8 @@ coverage_init (const char *filename) gcov_write_unsigned (bbg_file_stamp); } } + + dump_finish (pass_profile.pass.static_pass_number); } /* Performs file-level cleanup. Close notes file, generate coverage Index: ipa-inline.c =================================================================== --- ipa-inline.c (revision 201461) +++ ipa-inline.c (working copy) @@ -118,6 +118,9 @@ along with GCC; see the file COPYING3. If not see static int overall_size; static gcov_type max_count; +/* Global variable to denote if it is in ipa-inline pass. */ +bool is_in_ipa_inline = false; + /* Return false when inlining edge E would lead to violating limits on function unit growth or stack usage growth. @@ -1407,6 +1410,8 @@ inline_small_functions (void) int initial_size = 0; struct cgraph_node **order = XCNEWVEC (struct cgraph_node *, cgraph_n_nodes); + is_in_ipa_inline = true; + if (flag_indirect_inlining) new_indirect_edges.create (8); Index: ipa-inline.h =================================================================== --- ipa-inline.h (revision 201461) +++ ipa-inline.h (working copy) @@ -234,6 +234,7 @@ void clone_inlined_nodes (struct cgraph_edge *e, b extern int ncalls_inlined; extern int nfunctions_inlined; +extern bool is_in_ipa_inline; static inline struct inline_summary * inline_summary (struct cgraph_node *node) Index: testsuite/gcc.dg/pr40209.c =================================================================== --- testsuite/gcc.dg/pr40209.c (revision 201461) +++ testsuite/gcc.dg/pr40209.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O2 -fprofile-use" } */ +/* { dg-options "-O2 -fprofile-use -fopt-info" } */ void process(const char *s); Index: testsuite/gcc.dg/pr26570.c =================================================================== --- testsuite/gcc.dg/pr26570.c (revision 201461) +++ testsuite/gcc.dg/pr26570.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O2 -fprofile-generate -fprofile-use" } */ +/* { dg-options "-O2 -fprofile-generate -fprofile-use -fopt-info" } */ unsigned test (unsigned a, unsigned b) { Index: testsuite/gcc.dg/pr32773.c =================================================================== --- testsuite/gcc.dg/pr32773.c (revision 201461) +++ testsuite/gcc.dg/pr32773.c (working copy) @@ -1,6 +1,6 @@ /* { dg-do compile } */ -/* { dg-options "-O -fprofile-use" } */ -/* { dg-options "-O -m4 -fprofile-use" { target sh-*-* } } */ +/* { dg-options "-O -fprofile-use -fopt-info" } */ +/* { dg-options "-O -m4 -fprofile-use -fopt-info" { target sh-*-* } } */ void foo (int *p) { Index: testsuite/g++.dg/tree-ssa/dom-invalid.C =================================================================== --- testsuite/g++.dg/tree-ssa/dom-invalid.C (revision 201461) +++ testsuite/g++.dg/tree-ssa/dom-invalid.C (working copy) @@ -1,7 +1,7 @@ // PR tree-optimization/39557 // invalid post-dom info leads to infinite loop // { dg-do run } -// { dg-options "-Wall -fno-exceptions -O2 -fprofile-use -fno-rtti" } +// { dg-options "-Wall -fno-exceptions -O2 -fprofile-use -fopt-info -fno-rtti" } struct C {
This patch ports messages to the new dump framework, specifically those involving missing/mismatched/corrupted profile data, indirect call promotions performed, and inlines. For the inline messages, I ported Dehao's patch from the google branches that enables printing call chain information. I also fixed some issues with the dump_loc support for dumping out the new messages with location data, specifically to emit column numbers and to ensure the notes start on a new line. Finally, I made a couple enhancements to the new dump infrastructure to enable messages emitted from other parts of the compiler. There were two issues: 1) The coverage_init routine happens very early, before entering the pass manager that sets up the dumping. This isn't an issue in trunk yet, since there currently aren't any messages emitted during coverage_init (this showed up on the google branch where we emit messages for LIPO-specific profiles during this part of the compile), but it could be in the future. This was addressed by enabling dumping within coverage_init, and is modeled on how dumping is enabled after the pass manager during finish_optimization_passes. 2) Dumping was only enabled for passes marked with an optinfo_flag that isn't OPTGROUP_NONE. Currently optgroup flags are only setup for optimization groups in the categories IPA, LOOP, INLINE and VEC. What this means is that any dump messages added to a pass that isn't in one of these groups will silently be suppressed. The OPTGROUP setting is specified in opt_pass struct, and is also automatically set to OPTGROUP_IPA for any pass starting with "ipa-". What I did was to add a new optgroup macro, OPTGROUP_OTHER. This is enabled only under -fopt-info-optall, which is also the default for -fopt-info. That way dump messages can be emitted without the requirement that the pass be part of a group that can be emitted under a specific optimization group subset. When setting up the dumps, any pass that has OPTGROUP_NONE after examining the opt_pass struct and the pass name will use OPTGROUP_OTHER. This doesn't mean that the list of optgroups shouldn't be expanded, but rather adds a catch-all for passes that don't currently have or need to be emitted on their own as part of a new optgroup. Bootstrapped and tested on x86-64-unknown-linux-gnu. Ok for trunk? Thanks, Teresa 2013-08-06 Teresa Johnson <tejohnson@google.com> Dehao Chen <dehao@google.com> * dumpfile.c (dump_loc): Add column number to output, make newlines consistent. * dumpfile.h (OPTGROUP_OTHER): Add and enable under OPTGROUP_ALL. * ipa-inline-transform.c (clone_inlined_nodes): (cgraph_node_opt_info): New function. (cgraph_node_call_chain): Ditto. (dump_inline_decision): Ditto. (inline_call): Invoke dump_inline_decision. * doc/invoke.texi: Document optall -fopt-info flag. * profile.c (read_profile_edge_counts): Use new dump framework. (compute_branch_probabilities): Ditto. * passes.c (pass_manager::register_one_dump_file): Use OPTGROUP_OTHER when pass not in any opt group. * value-prof.c (check_counter): Use new dump framework. (find_func_by_funcdef_no): Ditto. (check_ic_target): Ditto. * coverage.c (get_coverage_counts): Ditto. (coverage_init): Setup new dump framework. * ipa-inline.c (inline_small_functions): Set is_in_ipa_inline. * ipa-inline.h (is_in_ipa_inline): Declare. * testsuite/gcc.dg/pr40209.c: Use -fopt-info. * testsuite/gcc.dg/pr26570.c: Ditto. * testsuite/gcc.dg/pr32773.c: Ditto. * testsuite/g++.dg/tree-ssa/dom-invalid.C (struct C): Ditto.