Fix memory leaks and use a pool_allocator
diff mbox

Message ID CAFiYyc2HnSMgWcPyrw6wKMKLqwZdSY0ZVDUR+WBLSz3gvN3aaQ@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener Nov. 9, 2015, 12:11 p.m. UTC
On Mon, Nov 9, 2015 at 12:22 PM, Martin Liška <mliska@suse.cz> wrote:
> Hi.
>
> This is follow-up of changes that Richi started on Friday.
>
> Patch can bootstrap on x86_64-linux-pc and regression tests are running.
>
> Ready for trunk?

        * tree-ssa-dom.c (free_edge_info): Make the function extern.
...
        * tree-ssa.h (free_edge_info): Declare function extern.

declare this in tree-ssa-threadupdate.h instead and renaming it to
sth less "public", like free_dom_edge_info.


@@ -1240,6 +1240,7 @@ lra_create_live_ranges_1 (bool all_p, bool dead_insn_p)
   dead_set = sparseset_alloc (max_regno);
   unused_set = sparseset_alloc (max_regno);
   curr_point = 0;
+  point_freq_vec.release ();
   point_freq_vec.create (get_max_uid () * 2);

a truncate (0) instead of a release () should be cheaper, avoiding the
re-allocation.

@@ -674,6 +674,10 @@ sra_deinitialize (void)
   assign_link_pool.release ();
   obstack_free (&name_obstack, NULL);

+  for (hash_map<tree, auto_vec<access_p> >::iterator it =
+       base_access_vec->begin (); it != base_access_vec->end (); ++it)
+    (*it).second.release ();
+
   delete base_access_vec;

I wonder if the better fix is to provide a proper free method for the hash_map?
A hash_map with 'auto_vec' looks suspicous - eventually a proper release
was intented here via default_hash_map_traits <>?

Anyway, most of the things above can be improved as followup of course.

Thanks,
Richard.

> Thanks,
> Martin

Comments

Trevor Saunders Nov. 9, 2015, 12:23 p.m. UTC | #1
On Mon, Nov 09, 2015 at 01:11:48PM +0100, Richard Biener wrote:
> On Mon, Nov 9, 2015 at 12:22 PM, Martin Liška <mliska@suse.cz> wrote:
> > Hi.
> >
> > This is follow-up of changes that Richi started on Friday.
> >
> > Patch can bootstrap on x86_64-linux-pc and regression tests are running.
> >
> > Ready for trunk?
> 
>         * tree-ssa-dom.c (free_edge_info): Make the function extern.
> ...
>         * tree-ssa.h (free_edge_info): Declare function extern.
> 
> declare this in tree-ssa-threadupdate.h instead and renaming it to
> sth less "public", like free_dom_edge_info.
> 
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index fff62de..eb6b7df 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -3161,6 +3161,8 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
>        set_used_flags (targets[i]);
>      }
> 
> +  temporaries.release ();
> +
>    set_used_flags (cond);
>    set_used_flags (x);
>    set_used_flags (y);
> @@ -3194,6 +3196,7 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
>      }
> 
>    num_updated_if_blocks++;
> +  targets.release ();
>    return TRUE;
> 
> suspiciously look like candidates for an auto_vec<> (didn't check).

I was about to say the same thing after a little checking (maybe the
region one in tree-ssa-threadupdate.c to, but didn't check that)

> 
> @@ -1240,6 +1240,7 @@ lra_create_live_ranges_1 (bool all_p, bool dead_insn_p)
>    dead_set = sparseset_alloc (max_regno);
>    unused_set = sparseset_alloc (max_regno);
>    curr_point = 0;
> +  point_freq_vec.release ();
>    point_freq_vec.create (get_max_uid () * 2);
> 
> a truncate (0) instead of a release () should be cheaper, avoiding the
> re-allocation.

yeah, or even change it to just grow the array, afaict it doesn't expect
the array to be cleared?

> @@ -674,6 +674,10 @@ sra_deinitialize (void)
>    assign_link_pool.release ();
>    obstack_free (&name_obstack, NULL);
> 
> +  for (hash_map<tree, auto_vec<access_p> >::iterator it =
> +       base_access_vec->begin (); it != base_access_vec->end (); ++it)
> +    (*it).second.release ();
> +
>    delete base_access_vec;
> 
> I wonder if the better fix is to provide a proper free method for the hash_map?
> A hash_map with 'auto_vec' looks suspicous - eventually a proper release
> was intented here via default_hash_map_traits <>?

in fact I would expect that already works, but apparently not, so I'd
say that's the bug.

Trev

> 
> Anyway, most of the things above can be improved as followup of course.
> 
> Thanks,
> Richard.
> 
> > Thanks,
> > Martin

Patch
diff mbox

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index fff62de..eb6b7df 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -3161,6 +3161,8 @@  noce_convert_multiple_sets (struct noce_if_info *if_info)
       set_used_flags (targets[i]);
     }

+  temporaries.release ();
+
   set_used_flags (cond);
   set_used_flags (x);
   set_used_flags (y);
@@ -3194,6 +3196,7 @@  noce_convert_multiple_sets (struct noce_if_info *if_info)
     }

   num_updated_if_blocks++;
+  targets.release ();
   return TRUE;

suspiciously look like candidates for an auto_vec<> (didn't check).