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

[Lehua Ding <lehua.ding@rivai.ai>]

Hi Vladimir,

Thank you so much for your review. Based on your comments, I feel like 
there are a lot of issues, especially the long compile time issue. So 
I'm going to reorganize and refactor the patches so that as many of them 
as possible can be reviewed separately. this way there will be fewer 
patches to support subreg in the end. I plan to split it into four 
separate patches like bellow. What do you think?

1. live_subreg problem
2. conflict_hard_regs check refactoring
3. use object instead of allocno to create copies
4. support subreg coalesce
    4.1 ira: Apply live_subreg data to ira
    4.2 lra: Apply live_subreg data to lra
    4.3 ira: Support subreg liveness track
    4.4 lra: Support subreg liveness track

So for the two patches about LRA, maybe you can stop review and wait for 
the revised patchs.

On 2023/11/17 5:13, Vladimir Makarov wrote:
> 
> 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.
> 
> 

-- 
Best,
Lehua (RiVAI)