[gcc-12] ira: Correct HONOR_REG_ALLOC_ORDER usage

Message ID CAFULd4Y=JZL=ycP6T1HfZkPum1vTZRrkBOANinGwT_sZ=vbZOA@mail.gmail.com
State New
Headers show
Series
  • [gcc-12] ira: Correct HONOR_REG_ALLOC_ORDER usage
Related show

Commit Message

Paul Richard Thomas via Gcc-patches Feb. 22, 2021, 4:06 p.m.
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  <ubizjak@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?

Uros.

Comments

Richard Biener Feb. 23, 2021, 7:48 a.m. | #1
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  <ubizjak@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?

Thanks,
Richard.

> Uros.

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
Paul Richard Thomas via Gcc-patches Feb. 23, 2021, 9:37 a.m. | #2
On Tue, Feb 23, 2021 at 8:48 AM Richard Biener <rguenther@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  <ubizjak@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.

Patch

diff --git a/gcc/defaults.h b/gcc/defaults.h
index 91216593e75..2af4add0c05 100644
--- a/gcc/defaults.h
+++ b/gcc/defaults.h
@@ -1047,7 +1047,11 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #endif
 
 #ifndef HONOR_REG_ALLOC_ORDER
-#define HONOR_REG_ALLOC_ORDER 0
+# if defined REG_ALLOC_ORDER
+#  define HONOR_REG_ALLOC_ORDER 1
+# else
+#  define HONOR_REG_ALLOC_ORDER 0
+# endif
 #endif
 
 /* EXIT_IGNORE_STACK should be nonzero if, when returning from a function,
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 062785af1e2..9a346555ec8 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -2050,7 +2050,10 @@  prologue and restoring it in the epilogue.  This discourages it from
 using call-saved registers.  If a machine wants to ensure that IRA
 allocates registers in the order given by REG_ALLOC_ORDER even if some
 call-saved registers appear earlier than call-used ones, then define this
-macro as a C expression to nonzero. Default is 0.
+macro as a C expression to nonzero.
+
+The default definition is @code{1} if @code{REG_ALLOC_ORDER} is defined;
+otherwise, it is @code{0}.
 @end defmac
 
 @defmac IRA_HARD_REGNO_ADD_COST_MULTIPLIER (@var{regno})
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 3b19e6f4281..1cb7fad7a46 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -1797,7 +1797,10 @@  prologue and restoring it in the epilogue.  This discourages it from
 using call-saved registers.  If a machine wants to ensure that IRA
 allocates registers in the order given by REG_ALLOC_ORDER even if some
 call-saved registers appear earlier than call-used ones, then define this
-macro as a C expression to nonzero. Default is 0.
+macro as a C expression to nonzero.
+
+The default definition is @code{1} if @code{REG_ALLOC_ORDER} is defined;
+otherwise, it is @code{0}.
 @end defmac
 
 @defmac IRA_HARD_REGNO_ADD_COST_MULTIPLIER (@var{regno})
diff --git a/gcc/ira-color.c b/gcc/ira-color.c
index 3d01c60800c..4586c9a1e08 100644
--- a/gcc/ira-color.c
+++ b/gcc/ira-color.c
@@ -4835,14 +4835,17 @@  ira_better_spill_reload_regno_p (int *regnos, int *other_regnos,
     return cost < other_cost;
   if (length != other_length)
     return length > other_length;
-#ifdef REG_ALLOC_ORDER
-  if (hard_regno >= 0 && other_hard_regno >= 0)
-    return (inv_reg_alloc_order[hard_regno]
-	    < inv_reg_alloc_order[other_hard_regno]);
-#else
-  if (call_used_count != other_call_used_count)
-    return call_used_count > other_call_used_count;
-#endif
+  if (HONOR_REG_ALLOC_ORDER)
+    {
+      if (hard_regno >= 0 && other_hard_regno >= 0)
+	return (inv_reg_alloc_order[hard_regno]
+		< inv_reg_alloc_order[other_hard_regno]);
+    }
+  else
+    {
+      if (call_used_count != other_call_used_count)
+	return call_used_count > other_call_used_count;
+    }
   return false;
 }