Re: [PATCH] use *_grow_cleared rather than *_grow on vec<bitmap_head>

[Richard Biener <rguenther@suse.de>]

On Thu, 28 Sep 2023, Jakub Jelinek wrote:

> On Thu, Sep 28, 2023 at 12:29:15PM +0200, Jakub Jelinek wrote:
> > On Thu, Sep 28, 2023 at 09:29:31AM +0000, Richard Biener wrote:
> > > > The following patch splits the bitmap_head class into a POD
> > > > struct bitmap_head_pod and bitmap_head derived from it with non-trivial
> > > > default constexpr constructor.  Most code should keep using bitmap_head
> > > > as before, bitmap_head_pod is there just for the cases where we want to
> > > > embed the bitmap head into a vector which we want to e.g. {quick,safe}_grow
> > > > and in a loop bitmap_initialize it afterwards (to avoid having to
> > > > {quick,safe}_grow_cleared them just to overwrite with bitmap_initialize).
> > > > The patch is larger than I hoped, because e.g. some code just used bitmap
> > > > and bitmap_head * or const_bitmap and const bitmap_head * interchangeably.
> > > > 
> > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > > 
> > > OK if there are no comments indicating otherwise.
> > 
> > A counter argument against this patch would be that it weakens the intent
> > to catch uses of uninitialized bitmaps for saving a few compile time cycles.
> > If one uses
> >   bitmap_head var;
> >   bitmap_initialize (&var, NULL);
> > etc., we spend those extra cycles to initialize it and nothing is told that
> > bitmap_initialize overwrites the whole var without ever using of any of its
> > elements, so DSE can't eliminate that.  And in the vec case which prompted
> > this patch it was about
> >   vec<bitmap_head> a;
> >   a.create (n);
> >   a.safe_grow (n); // vs. a.safe_grow_cleared (n);
> >   for (int i = 0; i < n; ++i)
> >     bitmap_initialize (&a[i], NULL);
> > When using bitmap_head_pod, one needs to ensure initialization without
> > help to catch such mistakes.
> 
> Here is the alternative patch which pays the small extra price while not
> undermining the checking.  Verified in all those places there is a loop
> doing bitmap_initialize immediately afterwards or worst case a few lines
> later.
> 
> With the static_assert uncommented, the remaining failures are poly_int
> related (supposedly gone with Richard S.'s poly_int patch) and the
> vect_unpromoted_value/ao_ref still unresolved cases.

OK, I like this better - it's only when we'd sparsely use the vec<>
that it's worth to delay initialization.

Richard.

> 2023-09-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* tree-ssa-loop-im.cc (tree_ssa_lim_initialize): Use quick_grow_cleared
> 	instead of quick_grow on vec<bitmap_head> members.
> 	* cfganal.cc (control_dependences::control_dependences): Likewise.
> 	* rtl-ssa/blocks.cc (function_info::build_info::build_info): Likewise.
> 	(function_info::place_phis): Use safe_grow_cleared instead of safe_grow
> 	on auto_vec<bitmap_head> vars.
> 	* tree-ssa-live.cc (compute_live_vars): Use quick_grow_cleared instead
> 	of quick_grow on vec<bitmap_head> var.
> 
> --- gcc/tree-ssa-loop-im.cc.jj	2023-09-28 12:06:03.527974171 +0200
> +++ gcc/tree-ssa-loop-im.cc	2023-09-28 12:38:07.028966742 +0200
> @@ -3496,13 +3496,13 @@ tree_ssa_lim_initialize (bool store_moti
>      (mem_ref_alloc (NULL, 0, UNANALYZABLE_MEM_ID));
>  
>    memory_accesses.refs_loaded_in_loop.create (number_of_loops (cfun));
> -  memory_accesses.refs_loaded_in_loop.quick_grow (number_of_loops (cfun));
> +  memory_accesses.refs_loaded_in_loop.quick_grow_cleared (number_of_loops (cfun));
>    memory_accesses.refs_stored_in_loop.create (number_of_loops (cfun));
> -  memory_accesses.refs_stored_in_loop.quick_grow (number_of_loops (cfun));
> +  memory_accesses.refs_stored_in_loop.quick_grow_cleared (number_of_loops (cfun));
>    if (store_motion)
>      {
>        memory_accesses.all_refs_stored_in_loop.create (number_of_loops (cfun));
> -      memory_accesses.all_refs_stored_in_loop.quick_grow
> +      memory_accesses.all_refs_stored_in_loop.quick_grow_cleared
>  						      (number_of_loops (cfun));
>      }
>  
> --- gcc/cfganal.cc.jj	2023-09-28 11:31:45.013870771 +0200
> +++ gcc/cfganal.cc	2023-09-28 12:37:34.302425957 +0200
> @@ -468,7 +468,7 @@ control_dependences::control_dependences
>  
>    bitmap_obstack_initialize (&m_bitmaps);
>    control_dependence_map.create (last_basic_block_for_fn (cfun));
> -  control_dependence_map.quick_grow (last_basic_block_for_fn (cfun));
> +  control_dependence_map.quick_grow_cleared (last_basic_block_for_fn (cfun));
>    for (int i = 0; i < last_basic_block_for_fn (cfun); ++i)
>      bitmap_initialize (&control_dependence_map[i], &m_bitmaps);
>    for (int i = 0; i < num_edges; ++i)
> --- gcc/rtl-ssa/blocks.cc.jj	2023-09-28 11:31:45.413865158 +0200
> +++ gcc/rtl-ssa/blocks.cc	2023-09-28 12:41:28.063145949 +0200
> @@ -57,7 +57,7 @@ function_info::build_info::build_info (u
>    // write to an entry before reading from it.  But poison the contents
>    // when checking, just to make sure we don't accidentally use an
>    // uninitialized value.
> -  bb_phis.quick_grow (num_bb_indices);
> +  bb_phis.quick_grow_cleared (num_bb_indices);
>    bb_mem_live_out.quick_grow (num_bb_indices);
>    bb_to_rpo.quick_grow (num_bb_indices);
>    if (flag_checking)
> @@ -614,7 +614,7 @@ function_info::place_phis (build_info &b
>  
>    // Calculate dominance frontiers.
>    auto_vec<bitmap_head> frontiers;
> -  frontiers.safe_grow (num_bb_indices);
> +  frontiers.safe_grow_cleared (num_bb_indices);
>    for (unsigned int i = 0; i < num_bb_indices; ++i)
>      bitmap_initialize (&frontiers[i], &bitmap_default_obstack);
>    compute_dominance_frontiers (frontiers.address ());
> @@ -626,7 +626,7 @@ function_info::place_phis (build_info &b
>    // they are live on entry to the corresponding block, but do not need
>    // phi nodes otherwise.
>    auto_vec<bitmap_head> unfiltered;
> -  unfiltered.safe_grow (num_bb_indices);
> +  unfiltered.safe_grow_cleared (num_bb_indices);
>    for (unsigned int i = 0; i < num_bb_indices; ++i)
>      bitmap_initialize (&unfiltered[i], &bitmap_default_obstack);
>  
> --- gcc/tree-ssa-live.cc.jj	2023-09-28 11:31:45.637862015 +0200
> +++ gcc/tree-ssa-live.cc	2023-09-28 12:38:25.590706289 +0200
> @@ -1361,7 +1361,7 @@ compute_live_vars (struct function *fn,
>       We then do a mostly classical bitmap liveness algorithm.  */
>  
>    active.create (last_basic_block_for_fn (fn));
> -  active.quick_grow (last_basic_block_for_fn (fn));
> +  active.quick_grow_cleared (last_basic_block_for_fn (fn));
>    for (int i = 0; i < last_basic_block_for_fn (fn); i++)
>      bitmap_initialize (&active[i], &bitmap_default_obstack);
>  
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)