Use exact=false for vec_safe_grow{,_cleared} functions.

Message ID dd57e0ab-72e8-4d83-b3e6-0b7c6728ff6d@suse.cz
State New
Headers show
Series
  • Use exact=false for vec_safe_grow{,_cleared} functions.
Related show

Commit Message

Martin Liška July 28, 2020, 10:23 a.m.
On 7/27/20 1:31 PM, Richard Biener wrote:
> No, add gro*_exact variants and replace existing ones with this, then switch

> to exact = false for the non-_exact variants.  Or add a exact=false argument

> to all of them and make all existing calls explicitly passing true.


-EBITLAZY

Anyway, I prepared a patch that adds exact=false for all the grow functions.
Apart from selective scheduling, all other consumers seems happy. And I removed
the artificial capacity growth calculations at places mentioned by Honza.

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

Ready to be installed?
Thanks,
Martin

Comments

Mike Crowe via Gcc-patches July 29, 2020, 1:47 p.m. | #1
On Tue, Jul 28, 2020 at 12:23 PM Martin Liška <mliska@suse.cz> wrote:
>

> On 7/27/20 1:31 PM, Richard Biener wrote:

> > No, add gro*_exact variants and replace existing ones with this, then switch

> > to exact = false for the non-_exact variants.  Or add a exact=false argument

> > to all of them and make all existing calls explicitly passing true.

>

> -EBITLAZY


Indeed you are - you're missing to update a lot of vec_safe_grow[_cleared]
calls.  Esp. the lto-streamer ones warrant a , true arg since we explicitly
streamed the number of elements.

How did you select the ones you updated and those you did not? (which
is why I asked for a boiler-plate replacement of , true first, then making
the parameter defaulted to false and adjusting those cases you found)

> Anyway, I prepared a patch that adds exact=false for all the grow functions.

> Apart from selective scheduling, all other consumers seems happy. And I removed

> the artificial capacity growth calculations at places mentioned by Honza.

>

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

>

> Ready to be installed?

> Thanks,

> Martin
Martin Liška July 30, 2020, 1:22 p.m. | #2
On 7/29/20 3:47 PM, Richard Biener wrote:
> On Tue, Jul 28, 2020 at 12:23 PM Martin Liška <mliska@suse.cz> wrote:

>>

>> On 7/27/20 1:31 PM, Richard Biener wrote:

>>> No, add gro*_exact variants and replace existing ones with this, then switch

>>> to exact = false for the non-_exact variants.  Or add a exact=false argument

>>> to all of them and make all existing calls explicitly passing true.

>>

>> -EBITLAZY

> 

> Indeed you are - you're missing to update a lot of vec_safe_grow[_cleared]

> calls.  Esp. the lto-streamer ones warrant a , true arg since we explicitly

> streamed the number of elements.


Yep, these should probably use exact allocation.

> 

> How did you select the ones you updated and those you did not?


Based on testing, I only kept changes in selective scheduling which caused
some ICEs.

(which
> is why I asked for a boiler-plate replacement of , true first, then making


This step would require to change about 250 places in the source code :(

> the parameter defaulted to false and adjusting those cases you found)


What about rather doing exact = true and changing the few places which don't need
it (as they make their of growth calculations)?

Martin

>> Anyway, I prepared a patch that adds exact=false for all the grow functions.

>> Apart from selective scheduling, all other consumers seems happy. And I removed

>> the artificial capacity growth calculations at places mentioned by Honza.

>>

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

>>

>> Ready to be installed?

>> Thanks,

>> Martin
Mike Crowe via Gcc-patches Aug. 3, 2020, 1:47 p.m. | #3
On Thu, Jul 30, 2020 at 3:22 PM Martin Liška <mliska@suse.cz> wrote:
>

> On 7/29/20 3:47 PM, Richard Biener wrote:

> > On Tue, Jul 28, 2020 at 12:23 PM Martin Liška <mliska@suse.cz> wrote:

> >>

> >> On 7/27/20 1:31 PM, Richard Biener wrote:

> >>> No, add gro*_exact variants and replace existing ones with this, then switch

> >>> to exact = false for the non-_exact variants.  Or add a exact=false argument

> >>> to all of them and make all existing calls explicitly passing true.

> >>

> >> -EBITLAZY

> >

> > Indeed you are - you're missing to update a lot of vec_safe_grow[_cleared]

> > calls.  Esp. the lto-streamer ones warrant a , true arg since we explicitly

> > streamed the number of elements.

>

> Yep, these should probably use exact allocation.

>

> >

> > How did you select the ones you updated and those you did not?

>

> Based on testing, I only kept changes in selective scheduling which caused

> some ICEs.

>

> (which

> > is why I asked for a boiler-plate replacement of , true first, then making

>

> This step would require to change about 250 places in the source code :(

>

> > the parameter defaulted to false and adjusting those cases you found)

>

> What about rather doing exact = true and changing the few places which don't need

> it (as they make their of growth calculations)?


But that would be inconsistent with the other 'exact' defaulted params in vec.h.

I think your patch points at the valid issue that the usually
defaulted 'exact' is
overridden in non _exact named functions without any way to change that
override.  But then we shouldn't change behavior when correcting that mistake.

Which is why my original suggestion added non-defaulted 'exact' paramters
to those places (to discover all places that need adjustment) and only after
that default it and remove the cases now equal to the defaulted case ...

Richard.

> Martin

>

> >> Anyway, I prepared a patch that adds exact=false for all the grow functions.

> >> Apart from selective scheduling, all other consumers seems happy. And I removed

> >> the artificial capacity growth calculations at places mentioned by Honza.

> >>

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

> >>

> >> Ready to be installed?

> >> Thanks,

> >> Martin

>

Patch

From 494c193931c42cbb2ca670bf2be203f7b70bd88d Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 27 Jul 2020 14:36:16 +0200
Subject: [PATCH] Use exact=false for vec_safe_grow{,_cleared} functions.

gcc/ChangeLog:

	* cfgrtl.c (rtl_create_basic_block): Do not calculate vector
	growth manually.
	* gimple.c (gimple_set_bb): Likewise.
	* tree-cfg.c (create_bb): Likewise.
	(move_block_to_fn): Likewise.
	* vec.h (vec_safe_grow): Add new argument exact with default
	value equal to false.
	(vec_safe_grow_cleared): Likewise.
	(vl_ptr>::safe_grow): Likewise.
	(vl_ptr>::safe_grow_cleared): Likewise.
	* symbol-summary.h: Do not call vec_safe_reserve +
	quick_grow_cleared.
---
 gcc/cfgrtl.c         |  8 ++------
 gcc/gimple.c         |  7 +------
 gcc/haifa-sched.c    |  2 +-
 gcc/sched-deps.c     |  2 +-
 gcc/sel-sched-ir.c   |  2 +-
 gcc/symbol-summary.h | 13 +++----------
 gcc/tree-cfg.c       | 21 ++++++---------------
 gcc/vec.h            | 27 +++++++++++++++------------
 8 files changed, 30 insertions(+), 52 deletions(-)

diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index 827e84a44dd..0e65537f255 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -374,12 +374,8 @@  rtl_create_basic_block (void *headp, void *endp, basic_block after)
   /* Grow the basic block array if needed.  */
   if ((size_t) last_basic_block_for_fn (cfun)
       >= basic_block_info_for_fn (cfun)->length ())
-    {
-      size_t new_size =
-	(last_basic_block_for_fn (cfun)
-	 + (last_basic_block_for_fn (cfun) + 3) / 4);
-      vec_safe_grow_cleared (basic_block_info_for_fn (cfun), new_size);
-    }
+    vec_safe_grow_cleared (basic_block_info_for_fn (cfun),
+			   last_basic_block_for_fn (cfun) + 1);
 
   n_basic_blocks_for_fn (cfun)++;
 
diff --git a/gcc/gimple.c b/gcc/gimple.c
index 10c562f4def..1175176aa70 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -1703,12 +1703,7 @@  gimple_set_bb (gimple *stmt, basic_block bb)
 	    vec_safe_length (label_to_block_map_for_fn (cfun));
 	  LABEL_DECL_UID (t) = uid = cfun->cfg->last_label_uid++;
 	  if (old_len <= (unsigned) uid)
-	    {
-	      unsigned new_len = 3 * uid / 2 + 1;
-
-	      vec_safe_grow_cleared (label_to_block_map_for_fn (cfun),
-				     new_len);
-	    }
+	    vec_safe_grow_cleared (label_to_block_map_for_fn (cfun), uid + 1);
 	}
 
       (*label_to_block_map_for_fn (cfun))[uid] = bb;
diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index 80687fb5359..e18a1de6855 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -9039,7 +9039,7 @@  extend_h_i_d (void)
   if (reserve > 0
       && ! h_i_d.space (reserve))
     {
-      h_i_d.safe_grow_cleared (3 * get_max_uid () / 2);
+      h_i_d.safe_grow_cleared (3 * get_max_uid () / 2, true);
       sched_extend_target ();
     }
 }
diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index 1bc75074e5d..d8b5c53c6a4 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -4072,7 +4072,7 @@  init_deps_data_vector (void)
 {
   int reserve = (sched_max_luid + 1 - h_d_i_d.length ());
   if (reserve > 0 && ! h_d_i_d.space (reserve))
-    h_d_i_d.safe_grow_cleared (3 * sched_max_luid / 2);
+    h_d_i_d.safe_grow_cleared (3 * sched_max_luid / 2, true);
 }
 
 /* If it is profitable to use them, initialize or extend (depending on
diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 4f1e4afdc4c..63fb6c671af 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -4209,7 +4209,7 @@  extend_insn_data (void)
         size = 3 * sched_max_luid / 2;
 
 
-      s_i_d.safe_grow_cleared (size);
+      s_i_d.safe_grow_cleared (size, true);
     }
 }
 
diff --git a/gcc/symbol-summary.h b/gcc/symbol-summary.h
index fa1df5c8015..a38eb1db778 100644
--- a/gcc/symbol-summary.h
+++ b/gcc/symbol-summary.h
@@ -354,11 +354,8 @@  public:
       id = this->m_symtab->assign_summary_id (node);
 
     if ((unsigned int)id >= m_vector->length ())
-      {
-	int newlen = this->m_symtab->cgraph_max_summary_id;
-	vec_safe_reserve (m_vector, newlen - m_vector->length ());
-	m_vector->quick_grow_cleared (newlen);
-      }
+      vec_safe_grow_cleared (m_vector,
+			     this->m_symtab->cgraph_max_summary_id);
 
     if ((*m_vector)[id] == NULL)
       (*m_vector)[id] = this->allocate_new ();
@@ -815,11 +812,7 @@  public:
       id = this->m_symtab->assign_summary_id (edge);
 
     if ((unsigned)id >= m_vector->length ())
-      {
-	int newlen = this->m_symtab->edges_max_summary_id;
-	m_vector->reserve (newlen - m_vector->length ());
-	m_vector->quick_grow_cleared (newlen);
-      }
+      vec_safe_grow_cleared (m_vector, this->m_symtab->edges_max_summary_id);
 
     if ((*m_vector)[id] == NULL)
       (*m_vector)[id] = this->allocate_new ();
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index b4d0c6db238..b79cf6c6d4c 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -681,12 +681,8 @@  create_bb (void *h, void *e, basic_block after)
   /* Grow the basic block array if needed.  */
   if ((size_t) last_basic_block_for_fn (cfun)
       == basic_block_info_for_fn (cfun)->length ())
-    {
-      size_t new_size =
-	(last_basic_block_for_fn (cfun)
-	 + (last_basic_block_for_fn (cfun) + 3) / 4);
-      vec_safe_grow_cleared (basic_block_info_for_fn (cfun), new_size);
-    }
+    vec_safe_grow_cleared (basic_block_info_for_fn (cfun),
+			   last_basic_block_for_fn (cfun) + 1);
 
   /* Add the newly created block to the array.  */
   SET_BASIC_BLOCK_FOR_FN (cfun, last_basic_block_for_fn (cfun), bb);
@@ -7097,7 +7093,7 @@  move_block_to_fn (struct function *dest_cfun, basic_block bb,
   edge_iterator ei;
   edge e;
   gimple_stmt_iterator si;
-  unsigned old_len, new_len;
+  unsigned old_len;
 
   /* Remove BB from dominance structures.  */
   delete_from_dominance_info (CDI_DOMINATORS, bb);
@@ -7133,10 +7129,8 @@  move_block_to_fn (struct function *dest_cfun, basic_block bb,
 
   old_len = vec_safe_length (cfg->x_basic_block_info);
   if ((unsigned) cfg->x_last_basic_block >= old_len)
-    {
-      new_len = cfg->x_last_basic_block + (cfg->x_last_basic_block + 3) / 4;
-      vec_safe_grow_cleared (cfg->x_basic_block_info, new_len);
-    }
+    vec_safe_grow_cleared (cfg->x_basic_block_info,
+			   cfg->x_last_basic_block + 1);
 
   (*cfg->x_basic_block_info)[bb->index] = bb;
 
@@ -7209,10 +7203,7 @@  move_block_to_fn (struct function *dest_cfun, basic_block bb,
 
 	  old_len = vec_safe_length (cfg->x_label_to_block_map);
 	  if (old_len <= (unsigned) uid)
-	    {
-	      new_len = 3 * uid / 2 + 1;
-	      vec_safe_grow_cleared (cfg->x_label_to_block_map, new_len);
-	    }
+	    vec_safe_grow_cleared (cfg->x_label_to_block_map, uid + 1);
 
 	  (*cfg->x_label_to_block_map)[uid] = bb;
 	  (*cfun->cfg->x_label_to_block_map)[uid] = NULL;
diff --git a/gcc/vec.h b/gcc/vec.h
index 3ad99b83690..a908d751ab7 100644
--- a/gcc/vec.h
+++ b/gcc/vec.h
@@ -723,11 +723,12 @@  vec_free (vec<T, A, vl_embed> *&v)
 /* Grow V to length LEN.  Allocate it, if necessary.  */
 template<typename T, typename A>
 inline void
-vec_safe_grow (vec<T, A, vl_embed> *&v, unsigned len CXX_MEM_STAT_INFO)
+vec_safe_grow (vec<T, A, vl_embed> *&v, unsigned len,
+	       bool exact = false CXX_MEM_STAT_INFO)
 {
   unsigned oldlen = vec_safe_length (v);
   gcc_checking_assert (len >= oldlen);
-  vec_safe_reserve_exact (v, len - oldlen PASS_MEM_STAT);
+  vec_safe_reserve (v, len - oldlen, exact PASS_MEM_STAT);
   v->quick_grow (len);
 }
 
@@ -735,10 +736,11 @@  vec_safe_grow (vec<T, A, vl_embed> *&v, unsigned len CXX_MEM_STAT_INFO)
 /* If V is NULL, allocate it.  Call V->safe_grow_cleared(LEN).  */
 template<typename T, typename A>
 inline void
-vec_safe_grow_cleared (vec<T, A, vl_embed> *&v, unsigned len CXX_MEM_STAT_INFO)
+vec_safe_grow_cleared (vec<T, A, vl_embed> *&v, unsigned len,
+		       bool exact = false CXX_MEM_STAT_INFO)
 {
   unsigned oldlen = vec_safe_length (v);
-  vec_safe_grow (v, len PASS_MEM_STAT);
+  vec_safe_grow (v, len, exact PASS_MEM_STAT);
   vec_default_construct (v->address () + oldlen, len - oldlen);
 }
 
@@ -748,9 +750,9 @@  vec_safe_grow_cleared (vec<T, A, vl_embed> *&v, unsigned len CXX_MEM_STAT_INFO)
 template<typename T>
 inline void
 vec_safe_grow_cleared (vec<T, va_heap, vl_ptr> *&v,
-		       unsigned len CXX_MEM_STAT_INFO)
+		       unsigned len, bool exact = false CXX_MEM_STAT_INFO)
 {
-  v->safe_grow_cleared (len PASS_MEM_STAT);
+  v->safe_grow_cleared (len, exact PASS_MEM_STAT);
 }
 
 /* If V does not have space for NELEMS elements, call
@@ -1460,8 +1462,8 @@  public:
   T *safe_push (const T &CXX_MEM_STAT_INFO);
   T &pop (void);
   void truncate (unsigned);
-  void safe_grow (unsigned CXX_MEM_STAT_INFO);
-  void safe_grow_cleared (unsigned CXX_MEM_STAT_INFO);
+  void safe_grow (unsigned, bool = false CXX_MEM_STAT_INFO);
+  void safe_grow_cleared (unsigned, bool = false CXX_MEM_STAT_INFO);
   void quick_grow (unsigned);
   void quick_grow_cleared (unsigned);
   void quick_insert (unsigned, const T &);
@@ -1887,11 +1889,11 @@  vec<T, va_heap, vl_ptr>::truncate (unsigned size)
 
 template<typename T>
 inline void
-vec<T, va_heap, vl_ptr>::safe_grow (unsigned len MEM_STAT_DECL)
+vec<T, va_heap, vl_ptr>::safe_grow (unsigned len, bool exact MEM_STAT_DECL)
 {
   unsigned oldlen = length ();
   gcc_checking_assert (oldlen <= len);
-  reserve_exact (len - oldlen PASS_MEM_STAT);
+  reserve (len - oldlen, exact PASS_MEM_STAT);
   if (m_vec)
     m_vec->quick_grow (len);
   else
@@ -1905,11 +1907,12 @@  vec<T, va_heap, vl_ptr>::safe_grow (unsigned len MEM_STAT_DECL)
 
 template<typename T>
 inline void
-vec<T, va_heap, vl_ptr>::safe_grow_cleared (unsigned len MEM_STAT_DECL)
+vec<T, va_heap, vl_ptr>::safe_grow_cleared (unsigned len, bool exact
+					    MEM_STAT_DECL)
 {
   unsigned oldlen = length ();
   size_t growby = len - oldlen;
-  safe_grow (len PASS_MEM_STAT);
+  safe_grow (len, exact PASS_MEM_STAT);
   if (growby != 0)
     vec_default_construct (address () + oldlen, growby);
 }
-- 
2.27.0