Re: [patch] Remove unused code from dse.c.
On 3/29/13, Jeff Law l...@redhat.com wrote: More correctly, it's been dead since an Oct 2008 patch from Richard Henderson which introduced set_mem_attrs_for_spill which effectively provides the alias analysis code with the information it needs to disambiguate stack slots from everything else without the callback. Lawrence, that's the critical bit of information you failed to provide. Sorry. Patch approved, please install. Committed. -- Lawrence Crowl
[patch] Remove unused code from dse.c.
This patch has been in the hash-table branch for months. I thought it didn't quite meet the criteria for obvious, but it's close. In dse.c, remove alias hash tables that are never set. Remove conditions that are then never true. Remove functions that are then never called. Remove variables that are then never read. Tested on x86-64. Index: gcc/ChangeLog 2013-03-29 Lawrence Crowl cr...@google.com * dse.c (clear_alias_sets): Remove never set. (disqualified_clear_alias_sets): Remove never set. (clear_alias_mode_pool): Remove never set. (dse_step0): Remove condition that is never true. (canon_address): Remove condition that is never true. (dse_step7): Remove condition that is never true. (rest_of_handle_dse): Remove condition that is never true. (rest_of_handle_dse::did_global): Remove never read from above. (dse_step2_spill): Remove never called from above. (dse_step5_spill): Remove never called from above. Index: gcc/dse.c === --- gcc/dse.c (revision 197226) +++ gcc/dse.c (working copy) @@ -572,20 +572,6 @@ static alloc_pool deferred_change_pool; static deferred_change_t deferred_change_list = NULL; -/* This are used to hold the alias sets of spill variables. Since - these are never aliased and there may be a lot of them, it makes - sense to treat them specially. This bitvector is only allocated in - calls from dse_record_singleton_alias_set which currently is only - made during reload1. So when dse is called before reload this - mechanism does nothing. */ - -static bitmap clear_alias_sets = NULL; - -/* The set of clear_alias_sets that have been disqualified because - there are loads or stores using a different mode than the alias set - was registered with. */ -static bitmap disqualified_clear_alias_sets = NULL; - /* The group that holds all of the clear_alias_sets. */ static group_info_t clear_alias_group; @@ -599,8 +585,6 @@ struct clear_alias_mode_holder enum machine_mode mode; }; -static alloc_pool clear_alias_mode_pool; - /* This is true except if cfun-stdarg -- i.e. we cannot do this for vararg functions because they play games with the frame. */ static bool stores_off_frame_dead_at_return; @@ -788,10 +772,7 @@ dse_step0 (void) init_alias_analysis (); - if (clear_alias_sets) -clear_alias_group = get_group_info (NULL); - else -clear_alias_group = NULL; + clear_alias_group = NULL; } @@ -1189,39 +1170,6 @@ canon_address (rtx mem, rtx expanded_address, address; int expanded; - /* Make sure that cselib is has initialized all of the operands of - the address before asking it to do the subst. */ - - if (clear_alias_sets) -{ - /* If this is a spill, do not do any further processing. */ - alias_set_type alias_set = MEM_ALIAS_SET (mem); - if (dump_file (dump_flags TDF_DETAILS)) - fprintf (dump_file, found alias set %d\n, (int) alias_set); - if (bitmap_bit_p (clear_alias_sets, alias_set)) - { - struct clear_alias_mode_holder *entry - = clear_alias_set_lookup (alias_set); - - /* If the modes do not match, we cannot process this set. */ - if (entry-mode != GET_MODE (mem)) - { - if (dump_file (dump_flags TDF_DETAILS)) - fprintf (dump_file, -disqualifying alias set %d, (%s) != (%s)\n, -(int) alias_set, GET_MODE_NAME (entry-mode), -GET_MODE_NAME (GET_MODE (mem))); - - bitmap_set_bit (disqualified_clear_alias_sets, alias_set); - return false; - } - - *alias_set_out = alias_set; - *group_id = clear_alias_group-id; - return true; - } -} - *alias_set_out = 0; cselib_lookup (mem_address, address_mode, 1, GET_MODE (mem)); @@ -2993,47 +2941,6 @@ dse_step2_nospill (void) } -/* Init the offset tables for the spill case. */ - -static bool -dse_step2_spill (void) -{ - unsigned int j; - group_info_t group = clear_alias_group; - bitmap_iterator bi; - - /* Position 0 is unused because 0 is used in the maps to mean - unused. */ - current_position = 1; - - if (dump_file (dump_flags TDF_DETAILS)) -{ - bitmap_print (dump_file, clear_alias_sets, - clear alias sets , \n); - bitmap_print (dump_file, disqualified_clear_alias_sets, - disqualified clear alias sets , \n); -} - - memset (group-offset_map_n, 0, sizeof(int) * group-offset_map_size_n); - memset (group-offset_map_p, 0, sizeof(int) * group-offset_map_size_p); - bitmap_clear (group-group_kill); - - /* Remove the disqualified positions from the store2_p set. */ - bitmap_and_compl_into (group-store2_p, disqualified_clear_alias_sets); - - /* We do not need to process the store2_n set because - alias_sets are
Re: [patch] Remove unused code from dse.c.
On 03/29/2013 02:24 AM, Lawrence Crowl wrote: This patch has been in the hash-table branch for months. I thought it didn't quite meet the criteria for obvious, but it's close. In dse.c, remove alias hash tables that are never set. Remove conditions that are then never true. Remove functions that are then never called. Remove variables that are then never read. Tested on x86-64. Index: gcc/ChangeLog 2013-03-29 Lawrence Crowl cr...@google.com * dse.c (clear_alias_sets): Remove never set. (disqualified_clear_alias_sets): Remove never set. (clear_alias_mode_pool): Remove never set. (dse_step0): Remove condition that is never true. (canon_address): Remove condition that is never true. (dse_step7): Remove condition that is never true. (rest_of_handle_dse): Remove condition that is never true. (rest_of_handle_dse::did_global): Remove never read from above. (dse_step2_spill): Remove never called from above. (dse_step5_spill): Remove never called from above. At what point did we stop setting clear_alias_sets? Was that intentional or not? If that was an intentional change, then this patchset is OK as-is. If losing the setting of clear_alias_sets was accidental, then I think we'll need to dig a bit further. The whole point of that code is to prevent wild reads from killing the spill slot stores being tracked. Of course with IRA/LRA all this may not be as important as it once was. Jeff
Re: [patch] Remove unused code from dse.c.
On 3/29/13, Jeff Law l...@redhat.com wrote: On 03/29/2013 02:24 AM, Lawrence Crowl wrote: This patch has been in the hash-table branch for months. I thought it didn't quite meet the criteria for obvious, but it's close. In dse.c, remove alias hash tables that are never set. Remove conditions that are then never true. Remove functions that are then never called. Remove variables that are then never read. Tested on x86-64. Index: gcc/ChangeLog 2013-03-29 Lawrence Crowl cr...@google.com * dse.c (clear_alias_sets): Remove never set. (disqualified_clear_alias_sets): Remove never set. (clear_alias_mode_pool): Remove never set. (dse_step0): Remove condition that is never true. (canon_address): Remove condition that is never true. (dse_step7): Remove condition that is never true. (rest_of_handle_dse): Remove condition that is never true. (rest_of_handle_dse::did_global): Remove never read from above. (dse_step2_spill): Remove never called from above. (dse_step5_spill): Remove never called from above. At what point did we stop setting clear_alias_sets? Was that intentional or not? I do not know the answer to either question. If that was an intentional change, then this patchset is OK as-is. If losing the setting of clear_alias_sets was accidental, then I think we'll need to dig a bit further. The whole point of that code is to prevent wild reads from killing the spill slot stores being tracked. Of course with IRA/LRA all this may not be as important as it once was. My view is that we have already lost the feature. The code that populates the set is gone. The remaining code has probably suffered bitrot because it is not being tested. Trying to recreate the population will probably result in inconsistencies anyway, necessitating a rewrite of the remaining code. So, the remaining code has little value, and might have negative value. -- Lawrence Crowl
Re: [patch] Remove unused code from dse.c.
On 03/29/2013 11:24 AM, Lawrence Crowl wrote: At what point did we stop setting clear_alias_sets? Was that intentional or not? I do not know the answer to either question. That's what needs to be determined before I'll approve. It means digging a bit. My view is that we have already lost the feature. The code that populates the set is gone. The remaining code has probably suffered bitrot because it is not being tested. Trying to recreate the population will probably result in inconsistencies anyway, necessitating a rewrite of the remaining code. So, the remaining code has little value, and might have negative value. But that doesn't mean dropping the code is the right thing to do. The right thing to do is see if the feature was dropped on purpose. If so, then we remove this dead code. If not, then we fix the real problem, namely the code was accidentally disabled (and add suitable tests to the suite to catch this kind of problem in the future). jeff
Re: [patch] Remove unused code from dse.c.
On Fri, Mar 29, 2013 at 6:29 PM, Jeff Law l...@redhat.com wrote: On 03/29/2013 11:24 AM, Lawrence Crowl wrote: At what point did we stop setting clear_alias_sets? Was that intentional or not? I do not know the answer to either question. That's what needs to be determined before I'll approve. It means digging a bit. My view is that we have already lost the feature. The code that populates the set is gone. The remaining code has probably suffered bitrot because it is not being tested. Trying to recreate the population will probably result in inconsistencies anyway, necessitating a rewrite of the remaining code. So, the remaining code has little value, and might have negative value. But that doesn't mean dropping the code is the right thing to do. The right thing to do is see if the feature was dropped on purpose. If so, then we remove this dead code. If not, then we fix the real problem, namely the code was accidentally disabled (and add suitable tests to the suite to catch this kind of problem in the future). It's left over cleanups from code removed last year: http://gcc.gnu.org/ml/gcc-patches/2012-03/msg01862.html I like the patch. Ciao! Steven
Re: [patch] Remove unused code from dse.c.
On 03/29/2013 12:12 PM, Steven Bosscher wrote: On Fri, Mar 29, 2013 at 6:29 PM, Jeff Law l...@redhat.com wrote: On 03/29/2013 11:24 AM, Lawrence Crowl wrote: At what point did we stop setting clear_alias_sets? Was that intentional or not? I do not know the answer to either question. That's what needs to be determined before I'll approve. It means digging a bit. My view is that we have already lost the feature. The code that populates the set is gone. The remaining code has probably suffered bitrot because it is not being tested. Trying to recreate the population will probably result in inconsistencies anyway, necessitating a rewrite of the remaining code. So, the remaining code has little value, and might have negative value. But that doesn't mean dropping the code is the right thing to do. The right thing to do is see if the feature was dropped on purpose. If so, then we remove this dead code. If not, then we fix the real problem, namely the code was accidentally disabled (and add suitable tests to the suite to catch this kind of problem in the future). It's left over cleanups from code removed last year: http://gcc.gnu.org/ml/gcc-patches/2012-03/msg01862.html More correctly, it's been dead since an Oct 2008 patch from Richard Henderson which introduced set_mem_attrs_for_spill which effectively provides the alias analysis code with the information it needs to disambiguate stack slots from everything else without the callback. Lawrence, that's the critical bit of information you failed to provide. Patch approved, please install. Thanks, Jeff