| Message ID | 87tx4iyvyi.fsf@e105548-lin.cambridge.arm.com |
|---|---|
| State | New |
| Headers | show |
On 09/08/14 09:26, Richard Sandiford wrote: > This patch adds a destructor to target_ira_int, so that the data structures > it points to are freed when the parent target_globals is freed. It fixes > a memory leak with non-default subtargets. > > Tested on x86_64-linux-gnu. OK to install? > > Thanks, > Richard > > > gcc/ > * ira.h (ira_finish_once): Delete. > * ira-int.h (target_ira_int::~target_ira_int): Declare. > (target_ira_int::free_ira_costs): Likewise. > (target_ira_int::free_register_move_costs): Likewise. > (ira_finish_costs_once): Delete. > * ira.c (free_register_move_costs): Replace with... > (target_ira_int::free_register_move_costs): ...this new function. > (target_ira_int::~target_ira_int): Define. > (ira_init): Call free_register_move_costs as a member function rather > than a global function. > (ira_finish_once): Delete. > * ira-costs.c (free_ira_costs): Replace with... > (target_ira_int::free_ira_costs): ...this new function. > (ira_init_costs): Call free_ira_costs as a member function rather > than a global function. > (ira_finish_costs_once): Delete. > * target-globals.c (target_globals::~target_globals): Call the > target_ira_int destructor. > * toplev.c: Include lra.h. > (finalize): Call lra_finish_once rather than ira_finish_once. Consider making target_ira_int a class. OK for the trunk. jeff
Jeff Law <law@redhat.com> writes: > On 09/08/14 09:26, Richard Sandiford wrote: >> This patch adds a destructor to target_ira_int, so that the data structures >> it points to are freed when the parent target_globals is freed. It fixes >> a memory leak with non-default subtargets. >> >> Tested on x86_64-linux-gnu. OK to install? >> >> Thanks, >> Richard >> >> >> gcc/ >> * ira.h (ira_finish_once): Delete. >> * ira-int.h (target_ira_int::~target_ira_int): Declare. >> (target_ira_int::free_ira_costs): Likewise. >> (target_ira_int::free_register_move_costs): Likewise. >> (ira_finish_costs_once): Delete. >> * ira.c (free_register_move_costs): Replace with... >> (target_ira_int::free_register_move_costs): ...this new function. >> (target_ira_int::~target_ira_int): Define. >> (ira_init): Call free_register_move_costs as a member function rather >> than a global function. >> (ira_finish_once): Delete. >> * ira-costs.c (free_ira_costs): Replace with... >> (target_ira_int::free_ira_costs): ...this new function. >> (ira_init_costs): Call free_ira_costs as a member function rather >> than a global function. >> (ira_finish_costs_once): Delete. >> * target-globals.c (target_globals::~target_globals): Call the >> target_ira_int destructor. >> * toplev.c: Include lra.h. >> (finalize): Call lra_finish_once rather than ira_finish_once. > Consider making target_ira_int a class. OK for the trunk. > > jeff I'd prefer to keep it as a struct if that's OK. At the moment these target structures are just collections of variables that are accessed directly, so it doesn't really feel like a proper OO class "yet". Also (more minor) changing it from a struct to a class would mean updating all references to the structure, to avoid the clang warning about mismatched tags. There would then be some weird-looking inconsistencies in the target-globals code. Thanks, Richard
On 09/12/14 01:25, Richard Sandiford wrote: > Jeff Law <law@redhat.com> writes: >> On 09/08/14 09:26, Richard Sandiford wrote: >>> This patch adds a destructor to target_ira_int, so that the data structures >>> it points to are freed when the parent target_globals is freed. It fixes >>> a memory leak with non-default subtargets. >>> >>> Tested on x86_64-linux-gnu. OK to install? >>> >>> Thanks, >>> Richard >>> >>> >>> gcc/ >>> * ira.h (ira_finish_once): Delete. >>> * ira-int.h (target_ira_int::~target_ira_int): Declare. >>> (target_ira_int::free_ira_costs): Likewise. >>> (target_ira_int::free_register_move_costs): Likewise. >>> (ira_finish_costs_once): Delete. >>> * ira.c (free_register_move_costs): Replace with... >>> (target_ira_int::free_register_move_costs): ...this new function. >>> (target_ira_int::~target_ira_int): Define. >>> (ira_init): Call free_register_move_costs as a member function rather >>> than a global function. >>> (ira_finish_once): Delete. >>> * ira-costs.c (free_ira_costs): Replace with... >>> (target_ira_int::free_ira_costs): ...this new function. >>> (ira_init_costs): Call free_ira_costs as a member function rather >>> than a global function. >>> (ira_finish_costs_once): Delete. >>> * target-globals.c (target_globals::~target_globals): Call the >>> target_ira_int destructor. >>> * toplev.c: Include lra.h. >>> (finalize): Call lra_finish_once rather than ira_finish_once. >> Consider making target_ira_int a class. OK for the trunk. >> >> jeff > > I'd prefer to keep it as a struct if that's OK. At the moment these > target structures are just collections of variables that are accessed > directly, so it doesn't really feel like a proper OO class "yet". > Also (more minor) changing it from a struct to a class would mean > updating all references to the structure, to avoid the clang warning > about mismatched tags. There would then be some weird-looking > inconsistencies in the target-globals code. It's OK with me. I just wanted you to consider it, clearly you have :-) jeff
Index: gcc/ira-costs.c =================================================================== --- gcc/ira-costs.c 2014-08-26 12:09:31.486621719 +0100 +++ gcc/ira-costs.c 2014-09-08 16:21:58.546319449 +0100 @@ -2047,21 +2047,21 @@ ira_init_costs_once (void) } /* Free allocated temporary cost vectors. */ -static void -free_ira_costs (void) +void +target_ira_int::free_ira_costs () { int i; - free (init_cost); - init_cost = NULL; + free (x_init_cost); + x_init_cost = NULL; for (i = 0; i < MAX_RECOG_OPERANDS; i++) { - free (op_costs[i]); - free (this_op_costs[i]); - op_costs[i] = this_op_costs[i] = NULL; + free (x_op_costs[i]); + free (x_this_op_costs[i]); + x_op_costs[i] = x_this_op_costs[i] = NULL; } - free (temp_costs); - temp_costs = NULL; + free (x_temp_costs); + x_temp_costs = NULL; } /* This is called each time register related information is @@ -2071,7 +2071,7 @@ ira_init_costs (void) { int i; - free_ira_costs (); + this_target_ira_int->free_ira_costs (); max_struct_costs_size = sizeof (struct costs) + sizeof (int) * (ira_important_classes_num - 1); /* Don't use ira_allocate because vectors live through several IRA @@ -2088,13 +2088,6 @@ ira_init_costs (void) temp_costs = (struct costs *) xmalloc (max_struct_costs_size); } -/* Function called once at the end of compiler work. */ -void -ira_finish_costs_once (void) -{ - free_ira_costs (); -} - /* Common initialization function for ira_costs and Index: gcc/ira-int.h =================================================================== --- gcc/ira-int.h 2014-08-26 12:09:21.218740214 +0100 +++ gcc/ira-int.h 2014-09-08 16:21:58.546319449 +0100 @@ -770,6 +770,11 @@ #define FOR_EACH_BIT_IN_MINMAX_SET(VEC, minmax_set_iter_next (&(ITER))) struct target_ira_int { + ~target_ira_int (); + + void free_ira_costs (); + void free_register_move_costs (); + /* Initialized once. It is a maximal possible size of the allocated struct costs. */ int x_max_struct_costs_size; @@ -1025,7 +1030,6 @@ extern void ira_destroy (void); /* ira-costs.c */ extern void ira_init_costs_once (void); extern void ira_init_costs (void); -extern void ira_finish_costs_once (void); extern void ira_costs (void); extern void ira_tune_allocno_costs (void); Index: gcc/ira.c =================================================================== --- gcc/ira.c 2014-08-29 10:05:41.590585897 +0100 +++ gcc/ira.c 2014-09-08 16:21:58.546319449 +0100 @@ -1673,40 +1673,46 @@ ira_init_once (void) /* Free ira_max_register_move_cost, ira_may_move_in_cost and ira_may_move_out_cost for each mode. */ -static void -free_register_move_costs (void) +void +target_ira_int::free_register_move_costs (void) { int mode, i; /* Reset move_cost and friends, making sure we only free shared table entries once. */ for (mode = 0; mode < MAX_MACHINE_MODE; mode++) - if (ira_register_move_cost[mode]) + if (x_ira_register_move_cost[mode]) { for (i = 0; - i < mode && (ira_register_move_cost[i] - != ira_register_move_cost[mode]); + i < mode && (x_ira_register_move_cost[i] + != x_ira_register_move_cost[mode]); i++) ; if (i == mode) { - free (ira_register_move_cost[mode]); - free (ira_may_move_in_cost[mode]); - free (ira_may_move_out_cost[mode]); + free (x_ira_register_move_cost[mode]); + free (x_ira_may_move_in_cost[mode]); + free (x_ira_may_move_out_cost[mode]); } } - memset (ira_register_move_cost, 0, sizeof ira_register_move_cost); - memset (ira_may_move_in_cost, 0, sizeof ira_may_move_in_cost); - memset (ira_may_move_out_cost, 0, sizeof ira_may_move_out_cost); + memset (x_ira_register_move_cost, 0, sizeof x_ira_register_move_cost); + memset (x_ira_may_move_in_cost, 0, sizeof x_ira_may_move_in_cost); + memset (x_ira_may_move_out_cost, 0, sizeof x_ira_may_move_out_cost); last_mode_for_init_move_cost = -1; } +target_ira_int::~target_ira_int () +{ + free_ira_costs (); + free_register_move_costs (); +} + /* This is called every time when register related information is changed. */ void ira_init (void) { - free_register_move_costs (); + this_target_ira_int->free_register_move_costs (); setup_reg_mode_hard_regset (); setup_alloc_regs (flag_omit_frame_pointer != 0); setup_class_subset_and_memory_move_costs (); @@ -1718,15 +1724,6 @@ ira_init (void) ira_init_costs (); } -/* Function called once at the end of compiler work. */ -void -ira_finish_once (void) -{ - ira_finish_costs_once (); - free_register_move_costs (); - lra_finish_once (); -} - #define ira_prohibited_mode_move_regs_initialized_p \ (this_target_ira_int->x_ira_prohibited_mode_move_regs_initialized_p) Index: gcc/ira.h =================================================================== --- gcc/ira.h 2014-08-29 10:05:41.590585897 +0100 +++ gcc/ira.h 2014-09-08 16:21:58.546319449 +0100 @@ -177,7 +177,6 @@ struct ira_reg_equiv_s extern void ira_init_once (void); extern void ira_init (void); -extern void ira_finish_once (void); extern void ira_setup_eliminable_regset (void); extern rtx ira_eliminate_regs (rtx, enum machine_mode); extern void ira_set_pseudo_classes (bool, FILE *); Index: gcc/target-globals.c =================================================================== --- gcc/target-globals.c 2014-09-05 16:15:31.769306144 +0100 +++ gcc/target-globals.c 2014-09-08 16:21:58.546319449 +0100 @@ -121,6 +121,7 @@ save_target_globals_default_opts () target_globals::~target_globals () { + ira_int->~target_ira_int (); /* default_target_globals points to static data so shouldn't be freed. */ if (this != &default_target_globals) { Index: gcc/toplev.c =================================================================== --- gcc/toplev.c 2014-09-05 15:06:42.917035061 +0100 +++ gcc/toplev.c 2014-09-08 16:21:58.546319449 +0100 @@ -55,6 +55,7 @@ Software Foundation; either version 3, o #include "params.h" #include "reload.h" #include "ira.h" +#include "lra.h" #include "dwarf2asm.h" #include "debug.h" #include "target.h" @@ -1887,7 +1888,7 @@ finalize (bool no_backend) g->get_passes ()->finish_optimization_passes (); - ira_finish_once (); + lra_finish_once (); } if (mem_report)