Re: [pushed][LRA][PR110372]: Refine reload pseudo class
On 7/12/23 12:22, Richard Sandiford wrote: Vladimir Makarov writes: On 7/12/23 06:07, Richard Sandiford wrote: Vladimir Makarov via Gcc-patches writes: diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc index 73fbef29912..2f95121df06 100644 --- a/gcc/lra-assigns.cc +++ b/gcc/lra-assigns.cc @@ -1443,10 +1443,11 @@ assign_by_spills (void) pass. Indicate that it is no longer spilled. */ bitmap_clear_bit (_spilled_pseudos, regno); assign_hard_regno (hard_regno, regno); - if (! reload_p) - /* As non-reload pseudo assignment is changed we - should reconsider insns referring for the - pseudo. */ + if (! reload_p || regno_allocno_class_array[regno] == ALL_REGS) Is this test meaningful on all targets? We have some for which GENERAL_REGS == ALL_REGS (e.g. nios2 and nvptx), so ALL_REGS can be a valid allocation class. Richard, thank you for the question. As I remember nvptx does not use IRA/LRA. I don't think it is a problem. For targets with GENERAL_REGS == ALL_REGS, it only results in one more insn processing on the next constraint sub-pass. Ah, ok, thanks. If there's no risk of cycling then I agree it doesn't matter. No. There is no additional risk of cycling as insn processing only starts after assigning hard reg to the reload pseudo and it can happens only once for the reload pseudo before spilling sub-pass.
Re: [pushed][LRA][PR110372]: Refine reload pseudo class
Vladimir Makarov writes: > On 7/12/23 06:07, Richard Sandiford wrote: >> Vladimir Makarov via Gcc-patches writes: >>> diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc >>> index 73fbef29912..2f95121df06 100644 >>> --- a/gcc/lra-assigns.cc >>> +++ b/gcc/lra-assigns.cc >>> @@ -1443,10 +1443,11 @@ assign_by_spills (void) >>> pass. Indicate that it is no longer spilled. */ >>> bitmap_clear_bit (_spilled_pseudos, regno); >>> assign_hard_regno (hard_regno, regno); >>> - if (! reload_p) >>> - /* As non-reload pseudo assignment is changed we >>> - should reconsider insns referring for the >>> - pseudo. */ >>> + if (! reload_p || regno_allocno_class_array[regno] == ALL_REGS) >> Is this test meaningful on all targets? We have some for which >> GENERAL_REGS == ALL_REGS (e.g. nios2 and nvptx), so ALL_REGS can >> be a valid allocation class. >> > Richard, thank you for the question. > > As I remember nvptx does not use IRA/LRA. > > I don't think it is a problem. For targets with GENERAL_REGS == > ALL_REGS, it only results in one more insn processing on the next > constraint sub-pass. Ah, ok, thanks. If there's no risk of cycling then I agree it doesn't matter. Richard > I could do more accurate solution but it would need introducing new data > (flags) for pseudos which I'd like to avoid.
Re: [pushed][LRA][PR110372]: Refine reload pseudo class
On 7/12/23 06:07, Richard Sandiford wrote: Vladimir Makarov via Gcc-patches writes: diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc index 73fbef29912..2f95121df06 100644 --- a/gcc/lra-assigns.cc +++ b/gcc/lra-assigns.cc @@ -1443,10 +1443,11 @@ assign_by_spills (void) pass. Indicate that it is no longer spilled. */ bitmap_clear_bit (_spilled_pseudos, regno); assign_hard_regno (hard_regno, regno); - if (! reload_p) - /* As non-reload pseudo assignment is changed we - should reconsider insns referring for the - pseudo. */ + if (! reload_p || regno_allocno_class_array[regno] == ALL_REGS) Is this test meaningful on all targets? We have some for which GENERAL_REGS == ALL_REGS (e.g. nios2 and nvptx), so ALL_REGS can be a valid allocation class. Richard, thank you for the question. As I remember nvptx does not use IRA/LRA. I don't think it is a problem. For targets with GENERAL_REGS == ALL_REGS, it only results in one more insn processing on the next constraint sub-pass. I could do more accurate solution but it would need introducing new data (flags) for pseudos which I'd like to avoid.
Re: [pushed][LRA][PR110372]: Refine reload pseudo class
Vladimir Makarov via Gcc-patches writes:
> The following patch solves
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110372
>
> The patch was successfully bootstrapped and tested on x86-64.
>
> commit 1f7e5a7b91862b999aab88ee0319052aaf00f0f1
> Author: Vladimir N. Makarov
> Date: Fri Jul 7 09:53:38 2023 -0400
>
> LRA: Refine reload pseudo class
>
> For given testcase a reload pseudo happened to occur only in reload
> insns created on one constraint sub-pass. Therefore its initial class
> (ALL_REGS) was not refined and the reload insns were not processed on
> the next constraint sub-passes. This resulted into the wrong insn.
>
> PR rtl-optimization/110372
>
> gcc/ChangeLog:
>
> * lra-assigns.cc (assign_by_spills): Add reload insns involving
> reload pseudos with non-refined class to be processed on the next
> sub-pass.
> * lra-constraints.cc (enough_allocatable_hard_regs_p): New func.
> (in_class_p): Use it.
> (print_curr_insn_alt): New func.
> (process_alt_operands): Use it. Improve debug info.
> (curr_insn_transform): Use print_curr_insn_alt. Refine reload
> pseudo class if it is not refined yet.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/i386/pr110372.c: New.
>
> diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc
> index 73fbef29912..2f95121df06 100644
> --- a/gcc/lra-assigns.cc
> +++ b/gcc/lra-assigns.cc
> @@ -1443,10 +1443,11 @@ assign_by_spills (void)
>pass. Indicate that it is no longer spilled. */
> bitmap_clear_bit (_spilled_pseudos, regno);
> assign_hard_regno (hard_regno, regno);
> - if (! reload_p)
> - /* As non-reload pseudo assignment is changed we
> -should reconsider insns referring for the
> -pseudo. */
> + if (! reload_p || regno_allocno_class_array[regno] == ALL_REGS)
Is this test meaningful on all targets? We have some for which
GENERAL_REGS == ALL_REGS (e.g. nios2 and nvptx), so ALL_REGS can
be a valid allocation class.
Thanks,
Richard
> + /* As non-reload pseudo assignment is changed we should
> +reconsider insns referring for the pseudo. Do the same if a
> +reload pseudo did not refine its class which can happens
> +when the pseudo occurs only in reload insns. */
> bitmap_set_bit (_pseudo_bitmap, regno);
> }
> }
> diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
> index 4dc2d70c402..123ff662cbc 100644
> --- a/gcc/lra-constraints.cc
> +++ b/gcc/lra-constraints.cc
> @@ -233,6 +233,34 @@ get_reg_class (int regno)
>return NO_REGS;
> }
>
> +/* Return true if REG_CLASS has enough allocatable hard regs to keep value of
> + REG_MODE. */
> +static bool
> +enough_allocatable_hard_regs_p (enum reg_class reg_class,
> + enum machine_mode reg_mode)
> +{
> + int i, j, hard_regno, class_size, nregs;
> +
> + if (hard_reg_set_subset_p (reg_class_contents[reg_class],
> lra_no_alloc_regs))
> +return false;
> + class_size = ira_class_hard_regs_num[reg_class];
> + for (i = 0; i < class_size; i++)
> +{
> + hard_regno = ira_class_hard_regs[reg_class][i];
> + nregs = hard_regno_nregs (hard_regno, reg_mode);
> + if (nregs == 1)
> + return true;
> + for (j = 0; j < nregs; j++)
> + if (TEST_HARD_REG_BIT (lra_no_alloc_regs, hard_regno + j)
> + || ! TEST_HARD_REG_BIT (reg_class_contents[reg_class],
> + hard_regno + j))
> + break;
> + if (j >= nregs)
> + return true;
> +}
> + return false;
> +}
> +
> /* Return true if REG satisfies (or will satisfy) reg class constraint
> CL. Use elimination first if REG is a hard register. If REG is a
> reload pseudo created by this constraints pass, assume that it will
> @@ -252,7 +280,6 @@ in_class_p (rtx reg, enum reg_class cl, enum reg_class
> *new_class,
>enum reg_class rclass, common_class;
>machine_mode reg_mode;
>rtx src;
> - int class_size, hard_regno, nregs, i, j;
>int regno = REGNO (reg);
>
>if (new_class != NULL)
> @@ -291,26 +318,7 @@ in_class_p (rtx reg, enum reg_class cl, enum reg_class
> *new_class,
>common_class = ira_reg_class_subset[rclass][cl];
>if (new_class != NULL)
> *new_class = common_class;
> - if (hard_reg_set_subset_p (reg_class_contents[common_class],
> - lra_no_alloc_regs))
> - return false;
> - /* Check that there are enough allocatable regs. */
> - class_size = ira_class_hard_regs_num[common_class];
> - for (i = 0; i < class_size; i++)
> - {
> - hard_regno = ira_class_hard_regs[common_class][i];
> - nregs = hard_regno_nregs (hard_regno, reg_mode);
> - if (nregs == 1)
[pushed][LRA][PR110372]: Refine reload pseudo class
The following patch solves
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110372
The patch was successfully bootstrapped and tested on x86-64.
commit 1f7e5a7b91862b999aab88ee0319052aaf00f0f1
Author: Vladimir N. Makarov
Date: Fri Jul 7 09:53:38 2023 -0400
LRA: Refine reload pseudo class
For given testcase a reload pseudo happened to occur only in reload
insns created on one constraint sub-pass. Therefore its initial class
(ALL_REGS) was not refined and the reload insns were not processed on
the next constraint sub-passes. This resulted into the wrong insn.
PR rtl-optimization/110372
gcc/ChangeLog:
* lra-assigns.cc (assign_by_spills): Add reload insns involving
reload pseudos with non-refined class to be processed on the next
sub-pass.
* lra-constraints.cc (enough_allocatable_hard_regs_p): New func.
(in_class_p): Use it.
(print_curr_insn_alt): New func.
(process_alt_operands): Use it. Improve debug info.
(curr_insn_transform): Use print_curr_insn_alt. Refine reload
pseudo class if it is not refined yet.
gcc/testsuite/ChangeLog:
* gcc.target/i386/pr110372.c: New.
diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc
index 73fbef29912..2f95121df06 100644
--- a/gcc/lra-assigns.cc
+++ b/gcc/lra-assigns.cc
@@ -1443,10 +1443,11 @@ assign_by_spills (void)
pass. Indicate that it is no longer spilled. */
bitmap_clear_bit (_spilled_pseudos, regno);
assign_hard_regno (hard_regno, regno);
- if (! reload_p)
- /* As non-reload pseudo assignment is changed we
- should reconsider insns referring for the
- pseudo. */
+ if (! reload_p || regno_allocno_class_array[regno] == ALL_REGS)
+ /* As non-reload pseudo assignment is changed we should
+ reconsider insns referring for the pseudo. Do the same if a
+ reload pseudo did not refine its class which can happens
+ when the pseudo occurs only in reload insns. */
bitmap_set_bit (_pseudo_bitmap, regno);
}
}
diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
index 4dc2d70c402..123ff662cbc 100644
--- a/gcc/lra-constraints.cc
+++ b/gcc/lra-constraints.cc
@@ -233,6 +233,34 @@ get_reg_class (int regno)
return NO_REGS;
}
+/* Return true if REG_CLASS has enough allocatable hard regs to keep value of
+ REG_MODE. */
+static bool
+enough_allocatable_hard_regs_p (enum reg_class reg_class,
+enum machine_mode reg_mode)
+{
+ int i, j, hard_regno, class_size, nregs;
+
+ if (hard_reg_set_subset_p (reg_class_contents[reg_class], lra_no_alloc_regs))
+return false;
+ class_size = ira_class_hard_regs_num[reg_class];
+ for (i = 0; i < class_size; i++)
+{
+ hard_regno = ira_class_hard_regs[reg_class][i];
+ nregs = hard_regno_nregs (hard_regno, reg_mode);
+ if (nregs == 1)
+ return true;
+ for (j = 0; j < nregs; j++)
+ if (TEST_HARD_REG_BIT (lra_no_alloc_regs, hard_regno + j)
+ || ! TEST_HARD_REG_BIT (reg_class_contents[reg_class],
+hard_regno + j))
+ break;
+ if (j >= nregs)
+ return true;
+}
+ return false;
+}
+
/* Return true if REG satisfies (or will satisfy) reg class constraint
CL. Use elimination first if REG is a hard register. If REG is a
reload pseudo created by this constraints pass, assume that it will
@@ -252,7 +280,6 @@ in_class_p (rtx reg, enum reg_class cl, enum reg_class *new_class,
enum reg_class rclass, common_class;
machine_mode reg_mode;
rtx src;
- int class_size, hard_regno, nregs, i, j;
int regno = REGNO (reg);
if (new_class != NULL)
@@ -291,26 +318,7 @@ in_class_p (rtx reg, enum reg_class cl, enum reg_class *new_class,
common_class = ira_reg_class_subset[rclass][cl];
if (new_class != NULL)
*new_class = common_class;
- if (hard_reg_set_subset_p (reg_class_contents[common_class],
- lra_no_alloc_regs))
- return false;
- /* Check that there are enough allocatable regs. */
- class_size = ira_class_hard_regs_num[common_class];
- for (i = 0; i < class_size; i++)
- {
- hard_regno = ira_class_hard_regs[common_class][i];
- nregs = hard_regno_nregs (hard_regno, reg_mode);
- if (nregs == 1)
- return true;
- for (j = 0; j < nregs; j++)
- if (TEST_HARD_REG_BIT (lra_no_alloc_regs, hard_regno + j)
- || ! TEST_HARD_REG_BIT (reg_class_contents[common_class],
- hard_regno + j))
- break;
- if (j >= nregs)
- return true;
- }
- return false;
+ return enough_allocatable_hard_regs_p (common_class, reg_mode);
}
}
@@ -2046,6 +2054,23 @@ update_and_check_small_class_inputs (int nop, int nalt,
return false;
}
+/* Print operand constraints for alternative ALT_NUMBER of the current
+ insn. */
+static void
+print_curr_insn_alt (int alt_number)
+{
+ for (int i = 0; i < curr_static_id->n_operands; i++)
+{
+ const char *p =
