[PATCH 2/6] ipa: Introduce ipa_cached_call_context

[PATCH 2/6] ipa: Introduce ipa_cached_call_context

[Martin Jambor]

Hi,

as we discussed with Honza on the mailin glist last week, making
cached call context structure distinct from the normal one may make it
clearer that the cached data need to be explicitely deallocated.

This implements does that division.  It is not mandatory for the overall
main goals of the patch set and can be dropped if deemed superfluous.

Last week I bootstrapped and tested (and LTO-bootstrapped) this patch
individually, this week I did so for the whole patch set.

OK for trunk?

Thanks,

Martin


gcc/ChangeLog:

2020-09-02  Martin Jambor  <mjambor@suse.cz>

	* ipa-fnsummary.h (ipa_cached_call_context): New forward declaration
	and class.
	(class ipa_call_context): Make friend ipa_cached_call_context.  Moved
	methods duplicate_from and release to it too.
	* ipa-fnsummary.c (ipa_call_context::duplicate_from): Moved to class
	ipa_cached_call_context.
	(ipa_call_context::release): Likewise, removed the parameter.
	* ipa-inline-analysis.c (node_context_cache_entry): Change the type of
	ctx to ipa_cached_call_context.
	(do_estimate_edge_time): Remove parameter from the call to
	ipa_cached_call_context::release.
---
 gcc/ipa-fnsummary.c       | 21 ++++++++-------------
 gcc/ipa-fnsummary.h       | 16 ++++++++++++++--
 gcc/ipa-inline-analysis.c |  4 ++--
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index d8b95aca307..aaccc203b3b 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -3284,7 +3284,7 @@ ipa_call_context::ipa_call_context (cgraph_node *node, clause_t possible_truths,
 /* Set THIS to be a duplicate of CTX.  Copy all relevant info.  */
 
 void
-ipa_call_context::duplicate_from (const ipa_call_context &ctx)
+ipa_cached_call_context::duplicate_from (const ipa_call_context &ctx)
 {
   m_node = ctx.m_node;
   m_possible_truths = ctx.m_possible_truths;
@@ -3354,24 +3354,19 @@ ipa_call_context::duplicate_from (const ipa_call_context &ctx)
   m_avals.m_known_value_ranges = vNULL;
 }
 
-/* Release memory used by known_vals/contexts/aggs vectors.
-   If ALL is true release also inline_param_summary.
-   This happens when context was previously duplicated to be stored
-   into cache.  */
+/* Release memory used by known_vals/contexts/aggs vectors.  and
+   inline_param_summary.  */
 
 void
-ipa_call_context::release (bool all)
+ipa_cached_call_context::release ()
 {
   /* See if context is initialized at first place.  */
   if (!m_node)
     return;
-  ipa_release_agg_values (m_avals.m_known_aggs, all);
-  if (all)
-    {
-      m_avals.m_known_vals.release ();
-      m_avals.m_known_contexts.release ();
-      m_inline_param_summary.release ();
-    }
+  ipa_release_agg_values (m_avals.m_known_aggs, true);
+  m_avals.m_known_vals.release ();
+  m_avals.m_known_contexts.release ();
+  m_inline_param_summary.release ();
 }
 
 /* Return true if CTX describes the same call context as THIS.  */
diff --git a/gcc/ipa-fnsummary.h b/gcc/ipa-fnsummary.h
index dfcde9f91b8..8e5659f62aa 100644
--- a/gcc/ipa-fnsummary.h
+++ b/gcc/ipa-fnsummary.h
@@ -287,6 +287,8 @@ public:
 			  ipa_call_summary *dst_data);
 };
 
+class ipa_cached_call_context;
+
 /* This object describe a context of call.  That is a summary of known
    information about its parameters.  Main purpose of this context is
    to give more realistic estimations of function runtime, size and
@@ -307,8 +309,6 @@ public:
 			       sreal *ret_time,
 			       sreal *ret_nonspecialized_time,
 			       ipa_hints *ret_hints);
-  void duplicate_from (const ipa_call_context &ctx);
-  void release (bool all = false);
   bool equal_to (const ipa_call_context &);
   bool exists_p ()
   {
@@ -329,6 +329,18 @@ private:
   /* Even after having calculated clauses, the information about argument
      values is used to resolve indirect calls.  */
   ipa_call_arg_values m_avals;
+
+  friend ipa_cached_call_context;
+};
+
+/* Variant of ipa_call_context that is stored in a cache over a longer period
+   of time.  */
+
+class ipa_cached_call_context : public ipa_call_context
+{
+public:
+  void duplicate_from (const ipa_call_context &ctx);
+  void release ();
 };
 
 extern fast_call_summary <ipa_call_summary *, va_heap> *ipa_call_summaries;
diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c
index d2ae8196d09..b7af77f7b9b 100644
--- a/gcc/ipa-inline-analysis.c
+++ b/gcc/ipa-inline-analysis.c
@@ -57,7 +57,7 @@ fast_call_summary<edge_growth_cache_entry *, va_heap> *edge_growth_cache = NULL;
 class node_context_cache_entry
 {
 public:
-  ipa_call_context ctx;
+  ipa_cached_call_context ctx;
   sreal time, nonspec_time;
   int size;
   ipa_hints hints;
@@ -226,7 +226,7 @@ do_estimate_edge_time (struct cgraph_edge *edge, sreal *ret_nonspec_time)
 	    node_context_cache_miss++;
 	  else
 	    node_context_cache_clear++;
-	  e->entry.ctx.release (true);
+	  e->entry.ctx.release ();
 	  ctx.estimate_size_and_time (&size, &min_size,
 				      &time, &nonspec_time, &hints);
 	  e->entry.size = size;
-- 
2.28.0

Re: [PATCH 2/6] ipa: Introduce ipa_cached_call_context

[Jan Hubicka]

> Hi,
> 
> as we discussed with Honza on the mailin glist last week, making
> cached call context structure distinct from the normal one may make it
> clearer that the cached data need to be explicitely deallocated.
> 
> This patch does that division.  It is not mandatory for the overall
> main goals of the patch set and can be dropped if deemed superfluous.
> 
> gcc/ChangeLog:
> 
> 2020-09-02  Martin Jambor  <mjambor@suse.cz>
> 
> 	* ipa-fnsummary.h (ipa_cached_call_context): New forward declaration
> 	and class.
> 	(class ipa_call_context): Make friend ipa_cached_call_context.  Moved
> 	methods duplicate_from and release to it too.
> 	* ipa-fnsummary.c (ipa_call_context::duplicate_from): Moved to class
> 	ipa_cached_call_context.
> 	(ipa_call_context::release): Likewise, removed the parameter.
> 	* ipa-inline-analysis.c (node_context_cache_entry): Change the type of
> 	ctx to ipa_cached_call_context.
> 	(do_estimate_edge_time): Remove parameter from the call to
> 	ipa_cached_call_context::release.
OK,
thanks
Honza

[PATCH 2/6] ipa: Introduce ipa_cached_call_context

[Martin Jambor]

Hi,

as we discussed with Honza on the mailin glist last week, making
cached call context structure distinct from the normal one may make it
clearer that the cached data need to be explicitely deallocated.

This patch does that division.  It is not mandatory for the overall
main goals of the patch set and can be dropped if deemed superfluous.

gcc/ChangeLog:

2020-09-02  Martin Jambor  <mjambor@suse.cz>

	* ipa-fnsummary.h (ipa_cached_call_context): New forward declaration
	and class.
	(class ipa_call_context): Make friend ipa_cached_call_context.  Moved
	methods duplicate_from and release to it too.
	* ipa-fnsummary.c (ipa_call_context::duplicate_from): Moved to class
	ipa_cached_call_context.
	(ipa_call_context::release): Likewise, removed the parameter.
	* ipa-inline-analysis.c (node_context_cache_entry): Change the type of
	ctx to ipa_cached_call_context.
	(do_estimate_edge_time): Remove parameter from the call to
	ipa_cached_call_context::release.
---
 gcc/ipa-fnsummary.c       | 21 ++++++++-------------
 gcc/ipa-fnsummary.h       | 16 ++++++++++++++--
 gcc/ipa-inline-analysis.c |  4 ++--
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index e8645aa0a1b..4ef7d2570e9 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -3329,7 +3329,7 @@ ipa_call_context::ipa_call_context (cgraph_node *node, clause_t possible_truths,
 /* Set THIS to be a duplicate of CTX.  Copy all relevant info.  */
 
 void
-ipa_call_context::duplicate_from (const ipa_call_context &ctx)
+ipa_cached_call_context::duplicate_from (const ipa_call_context &ctx)
 {
   m_node = ctx.m_node;
   m_possible_truths = ctx.m_possible_truths;
@@ -3399,24 +3399,19 @@ ipa_call_context::duplicate_from (const ipa_call_context &ctx)
   m_avals.m_known_value_ranges = vNULL;
 }
 
-/* Release memory used by known_vals/contexts/aggs vectors.
-   If ALL is true release also inline_param_summary.
-   This happens when context was previously duplicated to be stored
-   into cache.  */
+/* Release memory used by known_vals/contexts/aggs vectors.  and
+   inline_param_summary.  */
 
 void
-ipa_call_context::release (bool all)
+ipa_cached_call_context::release ()
 {
   /* See if context is initialized at first place.  */
   if (!m_node)
     return;
-  ipa_release_agg_values (m_avals.m_known_aggs, all);
-  if (all)
-    {
-      m_avals.m_known_vals.release ();
-      m_avals.m_known_contexts.release ();
-      m_inline_param_summary.release ();
-    }
+  ipa_release_agg_values (m_avals.m_known_aggs, true);
+  m_avals.m_known_vals.release ();
+  m_avals.m_known_contexts.release ();
+  m_inline_param_summary.release ();
 }
 
 /* Return true if CTX describes the same call context as THIS.  */
diff --git a/gcc/ipa-fnsummary.h b/gcc/ipa-fnsummary.h
index 6893858d18e..020a6f0425d 100644
--- a/gcc/ipa-fnsummary.h
+++ b/gcc/ipa-fnsummary.h
@@ -287,6 +287,8 @@ public:
 			  ipa_call_summary *dst_data);
 };
 
+class ipa_cached_call_context;
+
 /* This object describe a context of call.  That is a summary of known
    information about its parameters.  Main purpose of this context is
    to give more realistic estimations of function runtime, size and
@@ -307,8 +309,6 @@ public:
 			       sreal *ret_time,
 			       sreal *ret_nonspecialized_time,
 			       ipa_hints *ret_hints);
-  void duplicate_from (const ipa_call_context &ctx);
-  void release (bool all = false);
   bool equal_to (const ipa_call_context &);
   bool exists_p ()
   {
@@ -329,6 +329,18 @@ private:
   /* Even after having calculated clauses, the information about argument
      values is used to resolve indirect calls.  */
   ipa_call_arg_values m_avals;
+
+  friend ipa_cached_call_context;
+};
+
+/* Variant of ipa_call_context that is stored in a cache over a longer period
+   of time.  */
+
+class ipa_cached_call_context : public ipa_call_context
+{
+public:
+  void duplicate_from (const ipa_call_context &ctx);
+  void release ();
 };
 
 extern fast_call_summary <ipa_call_summary *, va_heap> *ipa_call_summaries;
diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c
index d2ae8196d09..b7af77f7b9b 100644
--- a/gcc/ipa-inline-analysis.c
+++ b/gcc/ipa-inline-analysis.c
@@ -57,7 +57,7 @@ fast_call_summary<edge_growth_cache_entry *, va_heap> *edge_growth_cache = NULL;
 class node_context_cache_entry
 {
 public:
-  ipa_call_context ctx;
+  ipa_cached_call_context ctx;
   sreal time, nonspec_time;
   int size;
   ipa_hints hints;
@@ -226,7 +226,7 @@ do_estimate_edge_time (struct cgraph_edge *edge, sreal *ret_nonspec_time)
 	    node_context_cache_miss++;
 	  else
 	    node_context_cache_clear++;
-	  e->entry.ctx.release (true);
+	  e->entry.ctx.release ();
 	  ctx.estimate_size_and_time (&size, &min_size,
 				      &time, &nonspec_time, &hints);
 	  e->entry.size = size;
-- 
2.28.0