Re: [PATCH V3 4/7] ira: Support subreg copy

[Vladimir Makarov <vmakarov@redhat.com>]

On 11/12/23 07:08, Lehua Ding wrote:
> This patch changes the previous way of creating a copy between allocnos to objects.
>
> gcc/ChangeLog:
>
> 	* ira-build.cc (find_allocno_copy): Removed.
> 	(find_object): New.
> 	(ira_create_copy): Adjust.
> 	(add_allocno_copy_to_list): Adjust.
> 	(swap_allocno_copy_ends_if_necessary): Adjust.
> 	(ira_add_allocno_copy): Adjust.
> 	(print_copy): Adjust.
> 	(print_allocno_copies): Adjust.
> 	(ira_flattening): Adjust.
> 	* ira-color.cc (INCLUDE_VECTOR): Include vector.
> 	(struct allocno_color_data): Adjust.
> 	(struct allocno_hard_regs_subnode): Adjust.
> 	(form_allocno_hard_regs_nodes_forest): Adjust.
> 	(update_left_conflict_sizes_p): Adjust.
> 	(struct update_cost_queue_elem): Adjust.
> 	(queue_update_cost): Adjust.
> 	(get_next_update_cost): Adjust.
> 	(update_costs_from_allocno): Adjust.
> 	(update_conflict_hard_regno_costs): Adjust.
> 	(assign_hard_reg): Adjust.
> 	(objects_conflict_by_live_ranges_p): New.
> 	(allocno_thread_conflict_p): Adjust.
> 	(object_thread_conflict_p): Ditto.
> 	(merge_threads): Ditto.
> 	(form_threads_from_copies): Ditto.
> 	(form_threads_from_bucket): Ditto.
> 	(form_threads_from_colorable_allocno): Ditto.
> 	(init_allocno_threads): Ditto.
> 	(add_allocno_to_bucket): Ditto.
> 	(delete_allocno_from_bucket): Ditto.
> 	(allocno_copy_cost_saving): Ditto.
> 	(color_allocnos): Ditto.
> 	(color_pass): Ditto.
> 	(update_curr_costs): Ditto.
> 	(coalesce_allocnos): Ditto.
> 	(ira_reuse_stack_slot): Ditto.
> 	(ira_initiate_assign): Ditto.
> 	(ira_finish_assign): Ditto.
> 	* ira-conflicts.cc (allocnos_conflict_for_copy_p): Ditto.
> 	(REG_SUBREG_P): Ditto.
> 	(subreg_move_p): New.
> 	(regs_non_conflict_for_copy_p): New.
> 	(subreg_reg_align_and_times_p): New.
> 	(process_regs_for_copy): Ditto.
> 	(add_insn_allocno_copies): Ditto.
> 	(propagate_copies): Ditto.
> 	* ira-emit.cc (add_range_and_copies_from_move_list): Ditto.
> 	* ira-int.h (struct ira_allocno_copy): Ditto.
> 	(ira_add_allocno_copy): Ditto.
> 	(find_object): Exported.
> 	(subreg_move_p): Exported.
> 	* ira.cc (print_redundant_copies): Exported.
>
> ---
>   gcc/ira-build.cc     | 154 +++++++-----
>   gcc/ira-color.cc     | 541 +++++++++++++++++++++++++++++++------------
>   gcc/ira-conflicts.cc | 173 +++++++++++---
>   gcc/ira-emit.cc      |  10 +-
>   gcc/ira-int.h        |  10 +-
>   gcc/ira.cc           |   5 +-
>   6 files changed, 646 insertions(+), 247 deletions(-)
The patch is mostly ok for me except that there are the same issues I 
mentioned in my 1st email. Not changing comments for functions with 
changed interface like function arg types and names (e.g. 
find_allocno_copy) is particularly bad.  It makes the comments confusing 
and wrong.  Also using just "adjust" in changelog entries is too brief.  
You should at least mention that function signature is changed.
> diff --git a/gcc/ira-build.cc b/gcc/ira-build.cc
> index a32693e69e4..13f0f7336ed 100644
> --- a/gcc/ira-build.cc
> +++ b/gcc/ira-build.cc
>
> diff --git a/gcc/ira-color.cc b/gcc/ira-color.cc
> index 8aed25144b9..099312bcdb3 100644
> --- a/gcc/ira-color.cc
> +++ b/gcc/ira-color.cc
> @@ -20,6 +20,7 @@ along with GCC; see the file COPYING3.  If not see
>   
....
>   
> -  ira_allocno_t next_thread_allocno;
> +  ira_object_t *next_thread_objects;
> +  /* The allocno all thread shared.  */
> +  ira_allocno_t first_thread_allocno;
> +  /* The offset start relative to the first_thread_allocno.  */
> +  int first_thread_offset;
> +  /* All allocnos belong to the thread.  */
> +  bitmap thread_allocnos;

It is better to use bitmap_head instead of bitmap.  It permits to avoid 
allocation of bitmap_head for bitmap.  There are many places when 
bitmap_head in you patches can be better used than bitmap (it is 
especially profitable if there is significant probability of empty bitmap).

Of  course the patch cab be committed when all the patches are approved 
and fixed.