Re: [PATCH] Convert more passes to new dump framework

[Teresa Johnson <tejohnson at google dot com>]

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



-- 
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413