On Tue, Feb 23, 2021 at 8:48 AM Richard Biener <rguent...@suse.de> wrote:
>
> On Mon, 22 Feb 2021, Uros Bizjak wrote:
>
> > The intention of HONOR_REG_ALLOC_ORDER is to ensure that IRA allocates
> > registers in the order given by REG_ALLOC_ORDER. However in
> > ira_better_spill_reload_regno_p, there is still a place where the
> > calculation depends on the presence of REG_ALLOC_ORDER, ignoring
> > HONOR_REG_ALLOC_ORDER macro altogether. The patch uses the correct macro
> > at this place.
> >
> > On the other hand, assign_hard_reg function respects HONOR_REG_ALLOC_ORDER,
> > but expects this macro to return 1 to avoid internal cost calculations.
> > As the macro is defined to 0 by default, it is expected that targets
> > redefine
> > HONOR_REG_ALLOC_ORDER to return nonzero value, even if REG_ALLOC_ORDER
> > is defined. This approach is prone to errors, so the patch defines
> > HONOR_REG_ALLOC_ORDER to 1 by default if REG_ALLOC_ORDER is defined.
> >
> > 2021-02-22 Uroš Bizjak <ubiz...@gmail.com>
> >
> > gcc/
> > * defaults.h (HONOR_REG_ALLOC_ORDER): If not defined,
> > define to 1 if REG_ALLOC_ORDER is defined.
> > * doc/tm.texi.in (HONOR_REG_ALLOC_ORDER):
> > Describe new default definition.
> > * doc/tm.texi: Regenerate.
> > * ira-color.c (ira_better_spill_reload_regno_p):
> > Use HONOR_REG_ALLOC_ORDER instead of REG_ALLOC_ORDER
> > to determine better spill reload regno.
> >
> > Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
> >
> > OK for gcc-12 when it opens?
>
> OK in case Vlad doesn't have a better suggestion or further comments.
>
> Do you have an idea for how many targets the changed default is an
> actual change?
Practically every target defines REG_ALLOC_ORDER, namely:
$ grep -R "define REG_ALLOC_ORDER" *
alpha/vms.h:#define REG_ALLOC_ORDER
alpha/alpha.h:#define REG_ALLOC_ORDER
arc/arc.h:#define REG_ALLOC_ORDER
arm/arm.h:#define REG_ALLOC_ORDER
avr/avr.h:#define REG_ALLOC_ORDER
bfin/bfin.h:#define REG_ALLOC_ORDER
c6x/c6x.h:#define REG_ALLOC_ORDER
cris/cris.h:#define REG_ALLOC_ORDER
csky/csky.h:#define REG_ALLOC_ORDER
epiphany/epiphany.h:#define REG_ALLOC_ORDER
frv/frv.h:#define REG_ALLOC_ORDER
h8300/h8300.h:#define REG_ALLOC_ORDER
i386/i386.h:#define REG_ALLOC_ORDER
ia64/ia64.h:#define REG_ALLOC_ORDER
iq2000/iq2000.h:#define REG_ALLOC_ORDER
m32c/m32c.h:#define REG_ALLOC_ORDER
m32r/m32r.h:#define REG_ALLOC_ORDER
m32r/m32r.h:#define REG_ALLOC_ORDER
m68k/m68k.h:#define REG_ALLOC_ORDER
mcore/mcore.h:#define REG_ALLOC_ORDER
mips/mips.h:#define REG_ALLOC_ORDER
mmix/mmix.h:#define REG_ALLOC_ORDER
mn10300/mn10300.h:#define REG_ALLOC_ORDER
msp430/msp430.h:#define REG_ALLOC_ORDER
nds32/nds32.h:#define REG_ALLOC_ORDER
nios2/nios2.h:#define REG_ALLOC_ORDER
or1k/or1k.h:#define REG_ALLOC_ORDER
pa/pa32-regs.h:#define REG_ALLOC_ORDER
pa/pa64-regs.h:#define REG_ALLOC_ORDER
pru/pru.h:#define REG_ALLOC_ORDER
riscv/riscv.h:#define REG_ALLOC_ORDER
rl78/rl78.h:#define REG_ALLOC_ORDER
rs6000/rs6000.h:#define REG_ALLOC_ORDER
rx/rx.h:#define REG_ALLOC_ORDER
s390/s390.h:#define REG_ALLOC_ORDER
sh/sh.h:#define REG_ALLOC_ORDER
sparc/sparc.h:#define REG_ALLOC_ORDER
stormy16/stormy16.h:#define REG_ALLOC_ORDER
tilegx/tilegx.h:#define REG_ALLOC_ORDER
tilepro/tilepro.h:#define REG_ALLOC_ORDER
v850/v850.h:#define REG_ALLOC_ORDER
visium/visium.h:#define REG_ALLOC_ORDER
xtensa/xtensa.h:#define REG_ALLOC_ORDER
while HONOR_REG_ALLOC_ORDER is defined by a few:
$ grep -R "define HONOR_REG_ALLOC_ORDER" *
arc/arc.h:#define HONOR_REG_ALLOC_ORDER 1
arm/arm.h:#define HONOR_REG_ALLOC_ORDER optimize_function_for_size_p (cfun)
nds32/nds32.h:#define HONOR_REG_ALLOC_ORDER optimize_size
nios2/nios2.h:#define HONOR_REG_ALLOC_ORDER (TARGET_HAS_CDX)
So, setting new HONOR_REG_ALLOC_ORDER default would have a noticeable
impact, mainly in the existing assing_hard_reg heuristics (please note
that patched condition in ira_better_spill_reload_regno with the new
default is a no-op for targets that don't define
HONOR_REG_ALLOC_ORDER, and a correction for targets that do).
Based on the comment in assign_hard_reg, the heuristic assumes that:
/* We don't care about giving callee saved registers to allocnos no
living through calls because call clobbered registers are
allocated first (it is usual practice to put them first in
REG_ALLOC_ORDER). */
and now for targets that define REG_ALLOC_ORDER (and don't define
HONOR_REG_ALLOC_ORDER) the new default *disables*
if (!HONOR_REG_ALLOC_ORDER)
{
if ((saved_nregs = calculate_saved_nregs (hard_regno, mode)) != 0)
/* We need to save/restore the hard register in
epilogue/prologue. Therefore we increase the cost. */
{
rclass = REGNO_REG_CLASS (hard_regno);
add_cost = ((ira_memory_move_cost[mode][rclass][0]
+ ira_memory_move_cost[mode][rclass][1])
* saved_nregs / hard_regno_nregs (hard_regno,
mode) - 1);
cost += add_cost;
full_cost += add_cost;
}
}
Assuming that REG_ALLOC_ORDER targets follow the usual practice and
put call-clobbered registers first (these regs don't have to be
saved/restored), then call-clobbered regs have precedence over
call-saved regs, and no additional cost has to be calculated.
Currently, the logic is incorrect and additional cost is calculated
for targets that define REG_ALLOC_ORDER and don't define
HONOR_REG_ALLOC_ORDER (atm, x86 also fits in this group).
Uros.
