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.

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).

[...]

> 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

Reply via email to