| Message ID | 201010130003.42922.ebotcazou@adacore.com |
|---|---|
| State | New |
| Headers | show |
On 10/12/2010 06:03 PM, Eric Botcazou wrote: > Hi, > > the newly added code in ira_tune_allocno_costs_and_cover_classes dealing with > unaligned registers: > > /* Some targets allow pseudos to be allocated to unaligned > sequences of hard registers. However, selecting an unaligned > sequence can unnecessarily restrict later allocations. So > increase the cost of unaligned hard regs to encourage the use > of aligned hard regs. */ > > makes an implicit assumption about the mapping of hard regs to indices within > classes: > > for (j = n - 1; j>= 0; j--) > { > if (j % nregs != 0) > { > regno = ira_non_ordered_class_hard_regs[cover_class][j]; > > The unalignment test is applied to the indices instead of the regnos so it > implicitly assumes that the mapping preserves the alignment. This is wrong > on the SPARC for GENERAL_REGS as the first register %g0 is hardwired to 0 so > the register with index 0 is %g1. This leads to quite poor allocation for > 32-bit code manipulating double words, for example the compiler itself. > > Bootstrapped/regtested on SPARC/Solaris 8, OK for mainline? > > Yes. Thanks for working on the PR, Eric. > 2010-10-12 Eric Botcazou<ebotcazou@adacore.com> > > PR rtl-optimization/45912 > * ira-costs.c (ira_tune_allocno_costs_and_cover_classes): Test the > regno of registers instead of their index to compute the alignment. > > > > 2010-10-12 Eric Botcazou<ebotcazou@adacore.com> > > * gcc.c-torture/execute/20101012-1.c: New test. > >
> > Bootstrapped/regtested on SPARC/Solaris 8, OK for mainline? > > Yes. Thanks for working on the PR, Eric. Thanks. I was wondering if ira_non_ordered_class_hard_regs isn't superfluous now that we iterate over the whole class. Can't we just do: for (j = n - 1; j >= 0; j--) { regno = ira_class_hard_regs[cover_class][j]; if ((regno % nregs) != 0) reg_costs[j] += ALLOCNO_FREQ (a); } and get rid of ira_non_ordered_class_hard_regs entirely? FWIW here are their contents for GENERAL_REGS on the SPARC: (gdb) p default_target_ira->x_ira_class_hard_regs[GENERAL_REGS] $5 = {1, 2, 3, 4, 29, 28, 27, 26, 25, 24, 15, 13, 12, 11, 10, 9, 8, 16, 17, 18, 19, 20, 21, 22, 23, 0 <repeats 77 times>} (gdb) p default_target_ira_int->x_ira_non_ordered_class_hard_regs[GENERAL_REGS] $6 = {1, 2, 3, 4, 8, 9, 10, 11, 12, 13, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 0 <repeats 77 times>}
On 10/13/2010 01:39 PM, Eric Botcazou wrote: >>> Bootstrapped/regtested on SPARC/Solaris 8, OK for mainline? >> Yes. Thanks for working on the PR, Eric. > Thanks. I was wondering if ira_non_ordered_class_hard_regs isn't superfluous > now that we iterate over the whole class. Can't we just do: > > for (j = n - 1; j>= 0; j--) > { > regno = ira_class_hard_regs[cover_class][j]; > if ((regno % nregs) != 0) > reg_costs[j] += ALLOCNO_FREQ (a); > } > Yes, we could remove it. But I think it is better to stay. Coming back to your previous patch, the problem occurs because ira_non_ordered_class_hard_regs contains only allocatable hard regs. If it contains all hard regs, there were no such PR. Although such change would need other changes (like supporting some consistency with ira_class_hard_reg_index and may be others). I still believe that alignment should be taken not from regno but by order number of the regno in its cover class because sometimes hard registers are enumerated as follows one class hard registers, some virtual hard regs, another class hard regs and for the second class hard regs, your approach will not work. So I'd like to consider your previous patch as a temporary solution (although some temporary solutions stay forever :). I think that solution I wrote here is better but not ideal too (for example in a situation when a cover class is union of some other classes). I'll put this work on my TODO list but honestly don't know when I start working on this. So far your patch solves the problem and should stay in the trunk. Thanks. > and get rid of ira_non_ordered_class_hard_regs entirely? FWIW here are their > contents for GENERAL_REGS on the SPARC: > > (gdb) p default_target_ira->x_ira_class_hard_regs[GENERAL_REGS] > $5 = {1, 2, 3, 4, 29, 28, 27, 26, 25, 24, 15, 13, 12, 11, 10, 9, 8, 16, 17, > 18, 19, 20, 21, 22, 23, 0<repeats 77 times>} > > (gdb) p > default_target_ira_int->x_ira_non_ordered_class_hard_regs[GENERAL_REGS] > $6 = {1, 2, 3, 4, 8, 9, 10, 11, 12, 13, 15, 16, 17, 18, 19, 20, 21, 22, 23, > 24, 25, 26, 27, 28, 29, 0<repeats 77 times>} >
> So I'd like to consider your previous patch as a temporary solution > (although some temporary solutions stay forever :). I think that > solution I wrote here is better but not ideal too (for example in a > situation when a cover class is union of some other classes). I'll put > this work on my TODO list but honestly don't know when I start working > on this. OK, thanks for the explanation. Btw, there is a strange loop in setup_class_hard_regs: for (i = 0; i < FIRST_PSEUDO_REGISTER; i++) { ira_non_ordered_class_hard_regs[cl][0] = -1; ira_class_hard_reg_index[cl][0] = -1; }
On 10/13/2010 05:38 PM, Eric Botcazou wrote: >> So I'd like to consider your previous patch as a temporary solution >> (although some temporary solutions stay forever :). I think that >> solution I wrote here is better but not ideal too (for example in a >> situation when a cover class is union of some other classes). I'll put >> this work on my TODO list but honestly don't know when I start working >> on this. > OK, thanks for the explanation. > > Btw, there is a strange loop in setup_class_hard_regs: > > for (i = 0; i< FIRST_PSEUDO_REGISTER; i++) > { > ira_non_ordered_class_hard_regs[cl][0] = -1; > ira_class_hard_reg_index[cl][0] = -1; > } > Obviously, it is a typo. I've just fixed it on the trunk. Thanks for pointing it out.
Index: ira-costs.c =================================================================== --- ira-costs.c (revision 165234) +++ ira-costs.c (working copy) @@ -1787,15 +1787,14 @@ ira_tune_allocno_costs_and_cover_classes if (min_cost != INT_MAX) ALLOCNO_COVER_CLASS_COST (a) = min_cost; - /* Some targets allow pseudos to be allocated to unaligned - sequences of hard registers. However, selecting an unaligned - sequence can unnecessarily restrict later allocations. So - increase the cost of unaligned hard regs to encourage the use - of aligned hard regs. */ + /* Some targets allow pseudos to be allocated to unaligned sequences + of hard registers. However, selecting an unaligned sequence can + unnecessarily restrict later allocations. So increase the cost of + unaligned hard regs to encourage the use of aligned hard regs. */ { - int nregs, index; + const int nregs = ira_reg_class_nregs[cover_class][ALLOCNO_MODE (a)]; - if ((nregs = ira_reg_class_nregs[cover_class][ALLOCNO_MODE (a)]) > 1) + if (nregs > 1) { ira_allocate_and_set_costs (&ALLOCNO_HARD_REG_COSTS (a), cover_class, @@ -1803,10 +1802,10 @@ ira_tune_allocno_costs_and_cover_classes reg_costs = ALLOCNO_HARD_REG_COSTS (a); for (j = n - 1; j >= 0; j--) { - if (j % nregs != 0) + regno = ira_non_ordered_class_hard_regs[cover_class][j]; + if ((regno % nregs) != 0) { - regno = ira_non_ordered_class_hard_regs[cover_class][j]; - index = ira_class_hard_reg_index[cover_class][regno]; + int index = ira_class_hard_reg_index[cover_class][regno]; ira_assert (index != -1); reg_costs[index] += ALLOCNO_FREQ (a); }
Hi, the newly added code in ira_tune_allocno_costs_and_cover_classes dealing with unaligned registers: /* Some targets allow pseudos to be allocated to unaligned sequences of hard registers. However, selecting an unaligned sequence can unnecessarily restrict later allocations. So increase the cost of unaligned hard regs to encourage the use of aligned hard regs. */ makes an implicit assumption about the mapping of hard regs to indices within classes: for (j = n - 1; j >= 0; j--) { if (j % nregs != 0) { regno = ira_non_ordered_class_hard_regs[cover_class][j]; The unalignment test is applied to the indices instead of the regnos so it implicitly assumes that the mapping preserves the alignment. This is wrong on the SPARC for GENERAL_REGS as the first register %g0 is hardwired to 0 so the register with index 0 is %g1. This leads to quite poor allocation for 32-bit code manipulating double words, for example the compiler itself. Bootstrapped/regtested on SPARC/Solaris 8, OK for mainline? 2010-10-12 Eric Botcazou <ebotcazou@adacore.com> PR rtl-optimization/45912 * ira-costs.c (ira_tune_allocno_costs_and_cover_classes): Test the regno of registers instead of their index to compute the alignment. 2010-10-12 Eric Botcazou <ebotcazou@adacore.com> * gcc.c-torture/execute/20101012-1.c: New test.