Re: [PATCH 05/14] Use summaries->get where possible. Small refactoring of multiple calls.

[Martin Liška <mliska at suse dot cz>]

On 06/07/2018 02:16 PM, Jan Hubicka wrote:
>>
>> gcc/ChangeLog:
>>
>> 2018-04-24  Martin Liska  <mliska@suse.cz>
>>
>> 	* ipa-fnsummary.c (dump_ipa_call_summary): Use ::get method.
>> 	(analyze_function_body): Extract multiple calls of get_create.
>> 	* ipa-inline-analysis.c (simple_edge_hints): Likewise.
>> 	* ipa-inline.c (recursive_inlining): Use ::get method.
>> 	* ipa-inline.h (estimate_edge_growth): Likewise.
>> ---
>>  gcc/ipa-fnsummary.c       | 14 +++++++-------
>>  gcc/ipa-inline-analysis.c |  2 +-
>>  gcc/ipa-inline.c          |  8 ++++----
>>  gcc/ipa-inline.h          |  7 +++----
>>  4 files changed, 15 insertions(+), 16 deletions(-)
>>
> 
>> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
>> index 8a6c5d0b5d8..e40b537bf61 100644
>> --- a/gcc/ipa-fnsummary.c
>> +++ b/gcc/ipa-fnsummary.c
>> @@ -850,7 +850,7 @@ dump_ipa_call_summary (FILE *f, int indent, struct cgraph_node *node,
>>  	  }
>>        if (!edge->inline_failed)
>>  	{
>> -	  ipa_fn_summary *s = ipa_fn_summaries->get_create (callee);
>> +	  ipa_fn_summary *s = ipa_fn_summaries->get (callee);
>>  	  fprintf (f, "%*sStack frame offset %i, callee self size %i,"
>>  		   " callee size %i\n",
>>  		   indent + 2, "",
>> @@ -2363,10 +2363,9 @@ analyze_function_body (struct cgraph_node *node, bool early)
>>  	    }
>>  	  free (body);
>>  	}
>> -      set_hint_predicate (&ipa_fn_summaries->get_create (node)->loop_iterations,
>> -			  loop_iterations);
>> -      set_hint_predicate (&ipa_fn_summaries->get_create (node)->loop_stride,
>> -			  loop_stride);
>> +      ipa_fn_summary *s = ipa_fn_summaries->get_create (node);
>> +      set_hint_predicate (&s->loop_iterations, loop_iterations);
>> +      set_hint_predicate (&s->loop_stride, loop_stride);
> 
> I think you already have pointer info initialized to ipa_fn_summaries->get (node), so just
> replace all uses pf ipa_fn_summaries in this function by that. It seems like not very careful
> transition (done pehraps by me :)
> 
> We may consider modifying our getters to make them pure for gcc so it will
> optimize some of those issues for us.  That would require some code refactoring
> as you have internal getter with bool parameter that also creates nodes (and
> thus is no longer pure)
>> diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c
>> index c4f904730e6..2e30a6d15ba 100644
>> --- a/gcc/ipa-inline-analysis.c
>> +++ b/gcc/ipa-inline-analysis.c
>> @@ -126,7 +126,7 @@ simple_edge_hints (struct cgraph_edge *edge)
>>  			    ? edge->caller->global.inlined_to : edge->caller);
>>    struct cgraph_node *callee = edge->callee->ultimate_alias_target ();
>>    if (ipa_fn_summaries->get_create (to)->scc_no
>> -      && ipa_fn_summaries->get_create (to)->scc_no
>> +      && ipa_fn_summaries->get (to)->scc_no
>>  	 == ipa_fn_summaries->get_create (callee)->scc_no
> 
> Please move ipa_fn_summaries->get_create (to)->scc_no out of the
> conditional and store the result, so we don't need to call it multiple times.
> I think it would be bug if summaries was not ready here, so it should be ->get
> only.
>> diff --git a/gcc/ipa-inline.h b/gcc/ipa-inline.h
>> index e8ae206d7b7..06bd38e551e 100644
>> --- a/gcc/ipa-inline.h
>> +++ b/gcc/ipa-inline.h
>> @@ -81,10 +81,9 @@ estimate_edge_size (struct cgraph_edge *edge)
>>  static inline int
>>  estimate_edge_growth (struct cgraph_edge *edge)
>>  {
>> -  gcc_checking_assert (ipa_call_summaries->get_create (edge)->call_stmt_size
>> -		       || !edge->callee->analyzed);
>> -  return (estimate_edge_size (edge)
>> -	  - ipa_call_summaries->get_create (edge)->call_stmt_size);
>> +  ipa_call_summary *s = ipa_call_summaries->get_create (edge);
>> +  gcc_checking_assert (s->call_stmt_size || !edge->callee->analyzed);
>> +  return (estimate_edge_size (edge) - s->call_stmt_size);
> 
> Also if get_create ever created new summary here, it would not have right sizes,
> so plase turn it into get here.
> 
> OK with those changes. (and if any of those trap, just add FIXME and we can deal with
> it incrementally).
> 
> Honza
> 

Ok, I'm doing that with patch that I attach.

Martin

>From f3a2951a3bbfb4091b7d4d141adb14a181eebc0a Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 8 Jun 2018 12:50:18 +0200
Subject: [PATCH] Replace some ::get_create with ::get in IPA inline.

gcc/ChangeLog:

2018-06-08  Martin Liska  <mliska@suse.cz>

	* ipa-inline-analysis.c (simple_edge_hints): Use ::get method.
	* ipa-inline.h (estimate_edge_growth): Likewise.
---
 gcc/ipa-inline-analysis.c | 8 ++++----
 gcc/ipa-inline.h          | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c
index 9a7267395ea..c781d368a8a 100644
--- a/gcc/ipa-inline-analysis.c
+++ b/gcc/ipa-inline-analysis.c
@@ -96,10 +96,10 @@ simple_edge_hints (struct cgraph_edge *edge)
   struct cgraph_node *to = (edge->caller->global.inlined_to
 			    ? edge->caller->global.inlined_to : edge->caller);
   struct cgraph_node *callee = edge->callee->ultimate_alias_target ();
-  if (ipa_fn_summaries->get_create (to)->scc_no
-      && ipa_fn_summaries->get (to)->scc_no
-	 == ipa_fn_summaries->get_create (callee)->scc_no
-      && !edge->recursive_p ())
+  int to_scc_no = ipa_fn_summaries->get (to)->scc_no;
+  int callee_scc_no = ipa_fn_summaries->get (callee)->scc_no;
+
+  if (to_scc_no && to_scc_no  == callee_scc_no && !edge->recursive_p ())
     hints |= INLINE_HINT_same_scc;
 
   if (callee->lto_file_data && edge->caller->lto_file_data
diff --git a/gcc/ipa-inline.h b/gcc/ipa-inline.h
index 15825bca820..02d6da06f6d 100644
--- a/gcc/ipa-inline.h
+++ b/gcc/ipa-inline.h
@@ -81,7 +81,7 @@ estimate_edge_size (struct cgraph_edge *edge)
 static inline int
 estimate_edge_growth (struct cgraph_edge *edge)
 {
-  ipa_call_summary *s = ipa_call_summaries->get_create (edge);
+  ipa_call_summary *s = ipa_call_summaries->get (edge);
   gcc_checking_assert (s->call_stmt_size || !edge->callee->analyzed);
   return (estimate_edge_size (edge) - s->call_stmt_size);
 }
-- 
2.17.0