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

[Xinliang David Li <davidxl at google dot com>]

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