diff mbox series

Use dump_asm_name for Callers/Calls in dump.

Message ID ac8cb79c-8897-bc0a-2c93-3e41c717a13e@suse.cz
State New
Headers show
Series Use dump_asm_name for Callers/Calls in dump. | expand

Commit Message

Martin Liška Jan. 7, 2020, 9:33 a.m. UTC 
Hi.

I would like to make cgraph_node names consistent in cgraph_node::dump.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

2020-01-02  Martin Liska  <mliska@suse.cz>

	* cgraph.c (cgraph_node::dump): Use systematically
	dump_asm_name.
---
  gcc/cgraph.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Martin Jambor Jan. 7, 2020, 10:12 a.m. UTC | #1 
Hi,

On Tue, Jan 07 2020, Martin Liška wrote:
> Hi.
>
> I would like to make cgraph_node names consistent in cgraph_node::dump.
>

I try to use just the symtab_order numbers whenever I can to avoid
confusion but - at least if we care about the non-asm name at all -
shouldn't the dump have both the name and asm_name?  If not, then I
guess get_dump_name should print asm name too to get consistency
everywhere in the IPA dumps not just the graph dump bit. 

Martin


> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> 2020-01-02  Martin Liska  <mliska@suse.cz>
>
> 	* cgraph.c (cgraph_node::dump): Use systematically
> 	dump_asm_name.
> ---
>   gcc/cgraph.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
>
> diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> index 13238d3d442..aa2c476842a 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -2036,7 +2036,7 @@ cgraph_node::dump (FILE *f)
>    profile_count sum = profile_count::zero ();
>    for (edge = callers; edge; edge = edge->next_caller)
>      {
> -      fprintf (f, "%s ", edge->caller->dump_name ());
> +      fprintf (f, "%s ", edge->caller->dump_asm_name ());
>        edge->dump_edge_flags (f);
>        if (edge->count.initialized_p ())
>  	sum += edge->count.ipa ();
> @@ -2045,7 +2045,7 @@ cgraph_node::dump (FILE *f)
>    fprintf (f, "\n  Calls: ");
>    for (edge = callees; edge; edge = edge->next_callee)
>      {
> -      fprintf (f, "%s ", edge->callee->dump_name ());
> +      fprintf (f, "%s ", edge->callee->dump_asm_name ());
>        edge->dump_edge_flags (f);
>      }
>    fprintf (f, "\n");
Martin Liška Jan. 7, 2020, 10:27 a.m. UTC | #2 
On 1/7/20 11:12 AM, Martin Jambor wrote:
> I try to use just the symtab_order numbers whenever I can to avoid
> confusion

Which is fine. Apparently there are just few usages of manual printing
of a symtab node and order like:

       fprintf (f,
	       "%*s%s/%i %s\n%*s  freq:%4.2f",
	       indent, "", callee->name (), callee->order,

I can replace these with symtab_node::dump_{asm_}name.

> but - at least if we care about the non-asm name at all -
> shouldn't the dump have both the name and asm_name?

That will probably make output of functions like cgraph_node::dump
harder to read.

> If not, then I
> guess get_dump_name should print asm name too to get consistency
> everywhere in the IPA dumps not just the graph dump bit.

One alternative would be to print always asm names? Or we can drive that
with a flag? Similar thing do all tools like objdump, as, .. They
have --demangle (--no-demangle) options.

Martin

> 
> Martin
Martin Liška Jan. 8, 2020, 9:54 a.m. UTC | #3 
On 1/7/20 11:27 AM, Martin Liška wrote:
> Which is fine. Apparently there are just few usages of manual printing
> of a symtab node and order like:
> 
>        fprintf (f,
>             "%*s%s/%i %s\n%*s  freq:%4.2f",
>             indent, "", callee->name (), callee->order,
> 
> I can replace these with symtab_node::dump_{asm_}name.

Hi.

I'm addressing this by the following refactoring patch.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin
Jan Hubicka Jan. 8, 2020, 10:08 a.m. UTC | #4 
> On 1/7/20 11:27 AM, Martin Liška wrote:
> > Which is fine. Apparently there are just few usages of manual printing
> > of a symtab node and order like:
> > 
> >        fprintf (f,
> >             "%*s%s/%i %s\n%*s  freq:%4.2f",
> >             indent, "", callee->name (), callee->order,
> > 
> > I can replace these with symtab_node::dump_{asm_}name.
> 
> Hi.
> 
> I'm addressing this by the following refactoring patch.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
This is OK.
The original reason for using DECL_NAME rather than DECL_ASSEMBLER_NAME
was that first is shorter, but also that DECL_ASSEMBLER_NAME may trigger
lazy assembler name computation that changes memory layout and let to
divergences in resulting code when compiling with and without
-fdump-ipa-all.

I guess for dumping we should use dump_name consistently unless we
really want to speak of the assembler symbol name (so I would suggest
fixing the uses of dump_asm_name in most cases to dump_name)

We should not use node->name - this is artifact from times we did not
have dump_name for dumping.

Honza
> Thanks,
> Martin

> From 45629d68ac4ad9dada74e8c14a10b89025f54762 Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Tue, 7 Jan 2020 12:37:42 +0100
> Subject: [PATCH] Replace node->name/node->order with node->dump_name.
> 
> gcc/ChangeLog:
> 
> 2020-01-07  Martin Liska  <mliska@suse.cz>
> 
> 	* ipa-fnsummary.c (dump_ipa_call_summary): Use symtab_node::dump_name.
> 	(ipa_call_context::estimate_size_and_time): Likewise.
> 	(inline_analyze_function): Likewise.
> 
> gcc/lto/ChangeLog:
> 
> 2020-01-07  Martin Liska  <mliska@suse.cz>
> 
> 	* lto-partition.c (lto_balanced_map): Use symtab_node::dump_name.
> ---
>  gcc/ipa-fnsummary.c     | 12 +++++-------
>  gcc/lto/lto-partition.c |  4 ++--
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
> index fa01cb6c083..7c0b6f98e25 100644
> --- a/gcc/ipa-fnsummary.c
> +++ b/gcc/ipa-fnsummary.c
> @@ -907,8 +907,8 @@ dump_ipa_call_summary (FILE *f, int indent, struct cgraph_node *node,
>        int i;
>  
>        fprintf (f,
> -	       "%*s%s/%i %s\n%*s  freq:%4.2f",
> -	       indent, "", callee->name (), callee->order,
> +	       "%*s%s %s\n%*s  freq:%4.2f",
> +	       indent, "", callee->dump_name (),
>  	       !edge->inline_failed
>  	       ? "inlined" : cgraph_inline_failed_string (edge-> inline_failed),
>  	       indent, "", edge->sreal_frequency ().to_double ());
> @@ -3505,9 +3505,8 @@ ipa_call_context::estimate_size_and_time (int *ret_size,
>    if (dump_file && (dump_flags & TDF_DETAILS))
>      {
>        bool found = false;
> -      fprintf (dump_file, "   Estimating body: %s/%i\n"
> -	       "   Known to be false: ", m_node->name (),
> -	       m_node->order);
> +      fprintf (dump_file, "   Estimating body: %s\n"
> +	       "   Known to be false: ", m_node->dump_name ());
>  
>        for (i = predicate::not_inlined_condition;
>  	   i < (predicate::first_dynamic_condition
> @@ -4034,8 +4033,7 @@ inline_analyze_function (struct cgraph_node *node)
>    push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>  
>    if (dump_file)
> -    fprintf (dump_file, "\nAnalyzing function: %s/%u\n",
> -	     node->name (), node->order);
> +    fprintf (dump_file, "\nAnalyzing function: %s\n", node->dump_name ());
>    if (opt_for_fn (node->decl, optimize) && !node->thunk.thunk_p)
>      inline_indirect_intraprocedural_analysis (node);
>    compute_fn_summary (node, false);
> diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c
> index 86b2eabe374..5b153c9759e 100644
> --- a/gcc/lto/lto-partition.c
> +++ b/gcc/lto/lto-partition.c
> @@ -734,10 +734,10 @@ lto_balanced_map (int n_lto_partitions, int max_partition_size)
>  	  best_varpool_pos = varpool_pos;
>  	}
>        if (dump_file)
> -	fprintf (dump_file, "Step %i: added %s/%i, size %i, "
> +	fprintf (dump_file, "Step %i: added %s, size %i, "
>  		 "cost %" PRId64 "/%" PRId64 " "
>  		 "best %" PRId64 "/%" PRId64", step %i\n", i,
> -		 order[i]->name (), order[i]->order,
> +		 order[i]->dump_name (),
>  		 partition->insns, cost, internal,
>  		 best_cost, best_internal, best_i);
>        /* Partition is too large, unwind into step when best cost was reached and
> -- 
> 2.24.1
>
Martin Liška Jan. 8, 2020, 12:20 p.m. UTC | #5 
On 1/8/20 11:08 AM, Jan Hubicka wrote:
>> On 1/7/20 11:27 AM, Martin Liška wrote:
>>> Which is fine. Apparently there are just few usages of manual printing
>>> of a symtab node and order like:
>>>
>>>         fprintf (f,
>>>              "%*s%s/%i %s\n%*s  freq:%4.2f",
>>>              indent, "", callee->name (), callee->order,
>>>
>>> I can replace these with symtab_node::dump_{asm_}name.
>>
>> Hi.
>>
>> I'm addressing this by the following refactoring patch.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
> This is OK.

Ok, I've just installed both patches.

> The original reason for using DECL_NAME rather than DECL_ASSEMBLER_NAME
> was that first is shorter, but also that DECL_ASSEMBLER_NAME may trigger
> lazy assembler name computation that changes memory layout and let to
> divergences in resulting code when compiling with and without
> -fdump-ipa-all.

Ah, that's new to me.

> 
> I guess for dumping we should use dump_name consistently unless we
> really want to speak of the assembler symbol name (so I would suggest
> fixing the uses of dump_asm_name in most cases to dump_name)

Well, I do prefer the asm names in cgraph dump files as heavy templated
C++ code generates function names that are much harder to grep. I know one
can use order to find these.

Example:



void Field<M, T, E>::clearDirty() const [with Mesh = UniformRectilinearMesh<MeshTraits<3, double, UniformRectilinearTag, CartesianTag, 3> >; T = double; EngineTag = MultiPatch<GridTag, Remote<Brick> >]/4590
vs.
_ZNK5FieldI22UniformRectilinearMeshI10MeshTraitsILi3Ed21UniformRectilinearTag12CartesianTagLi3EEEd10MultiPatchI7GridTag6RemoteI5BrickEEE10clearDirtyEv/4590

> 
> We should not use node->name - this is artifact from times we did not
> have dump_name for dumping.

I see still about 80 usages of that in dumps. I'll prepare a replacement
patch.

> 
> Honza
>> Thanks,
>> Martin
> 
>>  From 45629d68ac4ad9dada74e8c14a10b89025f54762 Mon Sep 17 00:00:00 2001
>> From: Martin Liska <mliska@suse.cz>
>> Date: Tue, 7 Jan 2020 12:37:42 +0100
>> Subject: [PATCH] Replace node->name/node->order with node->dump_name.
>>
>> gcc/ChangeLog:
>>
>> 2020-01-07  Martin Liska  <mliska@suse.cz>
>>
>> 	* ipa-fnsummary.c (dump_ipa_call_summary): Use symtab_node::dump_name.
>> 	(ipa_call_context::estimate_size_and_time): Likewise.
>> 	(inline_analyze_function): Likewise.
>>
>> gcc/lto/ChangeLog:
>>
>> 2020-01-07  Martin Liska  <mliska@suse.cz>
>>
>> 	* lto-partition.c (lto_balanced_map): Use symtab_node::dump_name.
>> ---
>>   gcc/ipa-fnsummary.c     | 12 +++++-------
>>   gcc/lto/lto-partition.c |  4 ++--
>>   2 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
>> index fa01cb6c083..7c0b6f98e25 100644
>> --- a/gcc/ipa-fnsummary.c
>> +++ b/gcc/ipa-fnsummary.c
>> @@ -907,8 +907,8 @@ dump_ipa_call_summary (FILE *f, int indent, struct cgraph_node *node,
>>         int i;
>>   
>>         fprintf (f,
>> -	       "%*s%s/%i %s\n%*s  freq:%4.2f",
>> -	       indent, "", callee->name (), callee->order,
>> +	       "%*s%s %s\n%*s  freq:%4.2f",
>> +	       indent, "", callee->dump_name (),
>>   	       !edge->inline_failed
>>   	       ? "inlined" : cgraph_inline_failed_string (edge-> inline_failed),
>>   	       indent, "", edge->sreal_frequency ().to_double ());
>> @@ -3505,9 +3505,8 @@ ipa_call_context::estimate_size_and_time (int *ret_size,
>>     if (dump_file && (dump_flags & TDF_DETAILS))
>>       {
>>         bool found = false;
>> -      fprintf (dump_file, "   Estimating body: %s/%i\n"
>> -	       "   Known to be false: ", m_node->name (),
>> -	       m_node->order);
>> +      fprintf (dump_file, "   Estimating body: %s\n"
>> +	       "   Known to be false: ", m_node->dump_name ());
>>   
>>         for (i = predicate::not_inlined_condition;
>>   	   i < (predicate::first_dynamic_condition
>> @@ -4034,8 +4033,7 @@ inline_analyze_function (struct cgraph_node *node)
>>     push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>>   
>>     if (dump_file)
>> -    fprintf (dump_file, "\nAnalyzing function: %s/%u\n",
>> -	     node->name (), node->order);
>> +    fprintf (dump_file, "\nAnalyzing function: %s\n", node->dump_name ());
>>     if (opt_for_fn (node->decl, optimize) && !node->thunk.thunk_p)
>>       inline_indirect_intraprocedural_analysis (node);
>>     compute_fn_summary (node, false);
>> diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c
>> index 86b2eabe374..5b153c9759e 100644
>> --- a/gcc/lto/lto-partition.c
>> +++ b/gcc/lto/lto-partition.c
>> @@ -734,10 +734,10 @@ lto_balanced_map (int n_lto_partitions, int max_partition_size)
>>   	  best_varpool_pos = varpool_pos;
>>   	}
>>         if (dump_file)
>> -	fprintf (dump_file, "Step %i: added %s/%i, size %i, "
>> +	fprintf (dump_file, "Step %i: added %s, size %i, "
>>   		 "cost %" PRId64 "/%" PRId64 " "
>>   		 "best %" PRId64 "/%" PRId64", step %i\n", i,
>> -		 order[i]->name (), order[i]->order,
>> +		 order[i]->dump_name (),
>>   		 partition->insns, cost, internal,
>>   		 best_cost, best_internal, best_i);
>>         /* Partition is too large, unwind into step when best cost was reached and
>> -- 
>> 2.24.1
>>
>
diff mbox series

Patch

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 13238d3d442..aa2c476842a 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -2036,7 +2036,7 @@  cgraph_node::dump (FILE *f)
   profile_count sum = profile_count::zero ();
   for (edge = callers; edge; edge = edge->next_caller)
     {
-      fprintf (f, "%s ", edge->caller->dump_name ());
+      fprintf (f, "%s ", edge->caller->dump_asm_name ());
       edge->dump_edge_flags (f);
       if (edge->count.initialized_p ())
 	sum += edge->count.ipa ();
@@ -2045,7 +2045,7 @@  cgraph_node::dump (FILE *f)
   fprintf (f, "\n  Calls: ");
   for (edge = callees; edge; edge = edge->next_callee)
     {
-      fprintf (f, "%s ", edge->callee->dump_name ());
+      fprintf (f, "%s ", edge->callee->dump_asm_name ());
       edge->dump_edge_flags (f);
     }
   fprintf (f, "\n");