This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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



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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]