| Message ID | 53727C74.2070502@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On 05/13/14 14:11, Sandra Loosemore wrote: > This patch is a follow-up to this thread from a few years ago: > > https://gcc.gnu.org/ml/gcc/2011-01/msg00093.html > https://gcc.gnu.org/ml/gcc/2011-01/msg00158.html > > As noted there, the current definition of ADJUST_REG_ALLOC_ORDER is > obsolete: > > (1) This hook is a holdover from the old pre-IRA register allocator and > it's not clear it does anything useful with IRA. > > (2) It's inconsistent with the current REG_ALLOC_ORDER definition which > is not just {0, 1, 2, ...} any more. > > I considered re-working the hook to jiggle the $24 ordering for MIPS16 > relative to the current REG_ALLOC_ORDER definition, but it wasn't clear > to me either where in the ordering it should go, or that it would be > worthwhile to try to tune this. Indeed, the CSiBE results for removing > it entirely are pretty much in the noise range, except that removing the > hack that is supposed to benefit MIPS16 resulted in more improvement for > that case than any of the others tested! Here are some numbers > comparing geomean for old and new with -Os for various combinations: > > default: 1062.3,1062.28 > -EL -mips16 -msoft-float: 758.624,758.551 > -EL -mips16: 763.398,763.321 > -EL -msoft-float: 1115.11,1115.09 > -EL: 1062.22,1062.2 > -EL -mabi=64: 1181.8,1181.81 > -mabi=64: 1181.93,1181.94 > -mips16 -msoft-float: 758.337,758.236 > -mips16: 763.11,763.011 > -EL -mabi=64 -msoft-float: 1218.11,1218.11 > -mabi=64 -msoft-float: 1218.24,1218.25 > -msoft-float: 1115.73,1115.71 > -EL -mmicromips -msoft-float: 792.314,792.32 > -EL -mnan=2008: 1062.22,1062.2 > -mnan=2008: 1062.3,1062.28 > > As for the invalid JALR patch I posted just now > https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01015.html > I had trouble regression-testing this patch on trunk due to various > other breakage in the past week, and ended up doing it on a 4.9.0 > checkout modified to support Mentor's extended set of mips-sde-elf > multilibs instead. That's also the source of the CSiBE numbers above. > Also, those numbers do include the JALR patch in the baseline. > > This patch did fix some of the dspr2-MULT.c and dspr2-MULTU.c test > failures that were reported in the original discussion, but I believe > the current XFAILs for those tests are still valid for the same reasons > listed in PR target/51729. > > So, this patch should be considered more of a code cleanup thing, than > either an optimization improvement or a test regression fix. OK to commit? > > -Sandra > > > 2014-05-13 Catherine Moore <clm@codesourcery.com> > Sandra Loosemore <sandra@codesourcery.com> > > gcc/ > * config/mips/mips.c (mips_order_regs_for_local_alloc): Delete. > * config/mips/mips.h (ADJUST_REG_ALLOC_ORDER): Delete. > * config/mips/mips-protos.h (mips_order_regs_for_local_alloc): Delete. OK for the trunk. jeff
Index: gcc/config/mips/mips.c =================================================================== --- gcc/config/mips/mips.c (revision 210372) +++ gcc/config/mips/mips.c (working copy) @@ -17509,28 +17509,6 @@ mips_conditional_register_usage (void) } } -/* When generating MIPS16 code, we want to allocate $24 (T_REG) before - other registers for instructions for which it is possible. This - encourages the compiler to use CMP in cases where an XOR would - require some register shuffling. */ - -void -mips_order_regs_for_local_alloc (void) -{ - int i; - - for (i = 0; i < FIRST_PSEUDO_REGISTER; i++) - reg_alloc_order[i] = i; - - if (TARGET_MIPS16) - { - /* It really doesn't matter where we put register 0, since it is - a fixed register anyhow. */ - reg_alloc_order[0] = 24; - reg_alloc_order[24] = 0; - } -} - /* Implement EH_USES. */ bool Index: gcc/config/mips/mips.h =================================================================== --- gcc/config/mips/mips.h (revision 210372) +++ gcc/config/mips/mips.h (working copy) @@ -2006,13 +2009,6 @@ enum reg_class 182,183,184,185,186,187 \ } -/* ADJUST_REG_ALLOC_ORDER is a macro which permits reg_alloc_order - to be rearranged based on a particular function. On the mips16, we - want to allocate $24 (T_REG) before other registers for - instructions for which it is possible. */ - -#define ADJUST_REG_ALLOC_ORDER mips_order_regs_for_local_alloc () - /* True if VALUE is an unsigned 6-bit number. */ #define UIMM6_OPERAND(VALUE) \ Index: gcc/config/mips/mips-protos.h =================================================================== --- gcc/config/mips/mips-protos.h (revision 210372) +++ gcc/config/mips/mips-protos.h (working copy) @@ -248,7 +248,6 @@ extern bool mips_expand_ext_as_unaligned extern bool mips_expand_ins_as_unaligned_store (rtx, rtx, HOST_WIDE_INT, HOST_WIDE_INT); extern bool mips_mem_fits_mode_p (enum machine_mode mode, rtx x); -extern void mips_order_regs_for_local_alloc (void); extern HOST_WIDE_INT mips_debugger_offset (rtx, HOST_WIDE_INT); extern void mips_push_asm_switch (struct mips_asm_switch *);
This patch is a follow-up to this thread from a few years ago: https://gcc.gnu.org/ml/gcc/2011-01/msg00093.html https://gcc.gnu.org/ml/gcc/2011-01/msg00158.html As noted there, the current definition of ADJUST_REG_ALLOC_ORDER is obsolete: (1) This hook is a holdover from the old pre-IRA register allocator and it's not clear it does anything useful with IRA. (2) It's inconsistent with the current REG_ALLOC_ORDER definition which is not just {0, 1, 2, ...} any more. I considered re-working the hook to jiggle the $24 ordering for MIPS16 relative to the current REG_ALLOC_ORDER definition, but it wasn't clear to me either where in the ordering it should go, or that it would be worthwhile to try to tune this. Indeed, the CSiBE results for removing it entirely are pretty much in the noise range, except that removing the hack that is supposed to benefit MIPS16 resulted in more improvement for that case than any of the others tested! Here are some numbers comparing geomean for old and new with -Os for various combinations: default: 1062.3,1062.28 -EL -mips16 -msoft-float: 758.624,758.551 -EL -mips16: 763.398,763.321 -EL -msoft-float: 1115.11,1115.09 -EL: 1062.22,1062.2 -EL -mabi=64: 1181.8,1181.81 -mabi=64: 1181.93,1181.94 -mips16 -msoft-float: 758.337,758.236 -mips16: 763.11,763.011 -EL -mabi=64 -msoft-float: 1218.11,1218.11 -mabi=64 -msoft-float: 1218.24,1218.25 -msoft-float: 1115.73,1115.71 -EL -mmicromips -msoft-float: 792.314,792.32 -EL -mnan=2008: 1062.22,1062.2 -mnan=2008: 1062.3,1062.28 As for the invalid JALR patch I posted just now https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01015.html I had trouble regression-testing this patch on trunk due to various other breakage in the past week, and ended up doing it on a 4.9.0 checkout modified to support Mentor's extended set of mips-sde-elf multilibs instead. That's also the source of the CSiBE numbers above. Also, those numbers do include the JALR patch in the baseline. This patch did fix some of the dspr2-MULT.c and dspr2-MULTU.c test failures that were reported in the original discussion, but I believe the current XFAILs for those tests are still valid for the same reasons listed in PR target/51729. So, this patch should be considered more of a code cleanup thing, than either an optimization improvement or a test regression fix. OK to commit? -Sandra 2014-05-13 Catherine Moore <clm@codesourcery.com> Sandra Loosemore <sandra@codesourcery.com> gcc/ * config/mips/mips.c (mips_order_regs_for_local_alloc): Delete. * config/mips/mips.h (ADJUST_REG_ALLOC_ORDER): Delete. * config/mips/mips-protos.h (mips_order_regs_for_local_alloc): Delete.