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 <mjam...@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 <tejohn...@google.com>
> >> Dehao Chen <de...@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
