Convert more passes to new dump framework

On Tue, Aug 6, 2013 at 9:29 AM, Teresa Johnson <tejohnson@google.com> wrote:
> 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.

New patch below that removes this global variable, and also outputs
the node->symbol.order (in square brackets after the function name so
as to not clutter it). Inline messages with profile data look look:

test.c:8:3: note: foobar [0] (99999000) inlined into foo [2] (1000)
with call count 99999000 (via inline instance bar [3] (99999000))

(without FDO the counts in parentheses and the call count would not be
included).

Ok for trunk?
Thanks,
Teresa

013-08-06  Teresa Johnson  <tejohnson@google.com>
            Dehao Chen  <dehao@google.com>

        * dumpfile.c (dump_loc): Output column number, make newlines consistent.
        * dumpfile.h (OPTGROUP_OTHER): Add and enable under OPTGROUP_ALL.
        * ipa-inline-transform.c (cgraph_node_opt_info): New function.
        (cgraph_node_call_chain): Ditto.
        (dump_inline_decision): Ditto.
        (inline_call): Invoke dump_inline_decision, new parameter.
        * 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 (recursive_inlining): New inline_call parameter.
        (inline_small_functions): Ditto.
        (flatten_function): Ditto.
        (ipa_inline): Ditto.
        (inline_always_inline_functions): Ditto.
        (early_inline_small_functions): Ditto.
        * ipa-inline.h: Ditto.

        * 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: Ditto.
        * testsuite/gcc.dg/inline-dump.c: New test.

+int bar(void) { return foo(); }
>
> Thanks,
> Teresa
>
>>
>> Thanks,
>>
>> Martin
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

Convert more passes to new dump framework

Commit Message

Comments

Patch