| Message ID | B5E67142681B53468FAF6B7C313565624F4ED3A9@hhmail02.hh.imgtec.org |
|---|---|
| State | New |
| Headers | show |
On May 20, 2016 4:58:47 PM GMT+02:00, Robert Suchanek <Robert.Suchanek@imgtec.com> wrote: s/splots/slots/ thanks,
On 05/20/2016 08:58 AM, Robert Suchanek wrote: > Hi, > > The patch changes the default behaviour of the direction in which > the local frame grows for MIPS16. > > The code size reduces by about 0.5% in average case for -Os, hence, > it is good to turn the option on by default. > > Ok to apply? > > Regards, > Robert > > gcc/ > > 2016-05-20 Matthew Fortune <matthew.fortune@imgtec.com> > > * config/mips/mips.h (FRAME_GROWS_DOWNWARD): Enable it > conditionally for MIPS16. > * config/mips/mips.opt: Add -mgrow-frame-downwards option. > Enable it by default for MIPS16. > * doc/invoke.texi: Document the option. This may be a stupid question, but what point is there in exposing this as an option to users? Users generally just want the compiler to emit good code when they compile with -O, not more individual optimization switches to twiddle. Is FRAME_GROWS_DOWNWARD likely to be so buggy or poorly tested that it's necessary to provide a way to turn it off? If we really must have this option.... > diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt > index 3b92ef5..53feb23 100644 > --- a/gcc/config/mips/mips.opt > +++ b/gcc/config/mips/mips.opt > @@ -447,3 +447,7 @@ Enum(mips_cb_setting) String(always) Value(MIPS_CB_ALWAYS) > minline-intermix > Target Report Var(TARGET_INLINE_INTERMIX) > Allow inlining even if the compression flags differ between caller and callee. > + > +mgrow-frame-downwards > +Target Report Var(TARGET_FRAME_GROWS_DOWNWARDS) Init(1) > +Change the behaviour to grow the frame downwards for MIPS16. British spelling of "behaviour" here. How about just "Grow the frame downwards for MIPS16." > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 2f6195e..6e5d620 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -17929,6 +17930,18 @@ vice-versa. When using this option it is necessary to protect functions > that cannot be compiled as MIPS16 with a @code{noinline} attribute to ensure > they are not inlined into a MIPS16 function. > > +@item -mgrow-frame-downwards > +@itemx -mno-grow-frame-downwards > +@opindex mgrow-frame-downwards > +Grow the local frame down (up) for MIPS16. > + > +Growing the frame downwards allows us to get spill slots created at the lowest s/allows us to get spill slots created/allows GCC to create spill slots/ > +address rather than the highest address in a local frame. The benefit of this > +is smaller code size as accessing spill splots closer to the stack pointer > +can be done using using 16-bit instructions. s/spill splots/spill slots/ But, this option description is so implementor-speaky that it just reinforces my thinking that it's likely to be uninteresting to users.... > + > +The option is enabled by default (to grow frame downwards) for MIPS16. > + > @item -mabi=32 > @itemx -mabi=o64 > @itemx -mabi=n32 > -Sandra
Sandra Loosemore <sandra@codesourcery.com> writes: > On 05/20/2016 08:58 AM, Robert Suchanek wrote: > > Hi, > > > > The patch changes the default behaviour of the direction in which the > > local frame grows for MIPS16. > > > > The code size reduces by about 0.5% in average case for -Os, hence, it > > is good to turn the option on by default. > > > > Ok to apply? > > > > Regards, > > Robert > > > > gcc/ > > > > 2016-05-20 Matthew Fortune <matthew.fortune@imgtec.com> > > > > * config/mips/mips.h (FRAME_GROWS_DOWNWARD): Enable it > > conditionally for MIPS16. > > * config/mips/mips.opt: Add -mgrow-frame-downwards option. > > Enable it by default for MIPS16. > > * doc/invoke.texi: Document the option. > > This may be a stupid question, but what point is there in exposing this > as an option to users? Users generally just want the compiler to emit Hi Sandra, Firstly, thanks for reviewing it is appreciated. There is some method to the madness in the sense that Robert and I have a reasonable number of patches that have been pending submission but have been released as part of toolchains from Imagination. We figured it would be best to post them as is and then have this kind of discussion in the open about what to keep and what to change so I expect there to be a few more things like this to review. I'm likely to propose changes myself too with my upstream hat on. > good code when they compile with -O, not more individual optimization > switches to twiddle. Agreed. > Is FRAME_GROWS_DOWNWARD likely to be so buggy or > poorly tested that it's necessary to provide a way to turn it off? No. I have no objection to removing this option. We have had it for a while in our sources and found no need to advise anyone to turn it off so hardwiring FRAME_GROWS_DOWNWARD for mips16 looks good. Thanks, Matthew > If we really must have this option.... > > > diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt index > > 3b92ef5..53feb23 100644 > > --- a/gcc/config/mips/mips.opt > > +++ b/gcc/config/mips/mips.opt > > @@ -447,3 +447,7 @@ Enum(mips_cb_setting) String(always) > Value(MIPS_CB_ALWAYS) > > minline-intermix > > Target Report Var(TARGET_INLINE_INTERMIX) > > Allow inlining even if the compression flags differ between caller > and callee. > > + > > +mgrow-frame-downwards > > +Target Report Var(TARGET_FRAME_GROWS_DOWNWARDS) Init(1) Change the > > +behaviour to grow the frame downwards for MIPS16. > > British spelling of "behaviour" here. How about just "Grow the frame > downwards for MIPS16." > > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index > > 2f6195e..6e5d620 100644 > > --- a/gcc/doc/invoke.texi > > +++ b/gcc/doc/invoke.texi > > @@ -17929,6 +17930,18 @@ vice-versa. When using this option it is > necessary to protect functions > > that cannot be compiled as MIPS16 with a @code{noinline} attribute > to ensure > > they are not inlined into a MIPS16 function. > > > > +@item -mgrow-frame-downwards > > +@itemx -mno-grow-frame-downwards > > +@opindex mgrow-frame-downwards > > +Grow the local frame down (up) for MIPS16. > > + > > +Growing the frame downwards allows us to get spill slots created at > > +the lowest > > s/allows us to get spill slots created/allows GCC to create spill slots/ > > > +address rather than the highest address in a local frame. The > > +benefit of this is smaller code size as accessing spill splots closer > > +to the stack pointer can be done using using 16-bit instructions. > > s/spill splots/spill slots/ > > But, this option description is so implementor-speaky that it just > reinforces my thinking that it's likely to be uninteresting to users.... > > > + > > +The option is enabled by default (to grow frame downwards) for > MIPS16. > > + > > @item -mabi=32 > > @itemx -mabi=o64 > > @itemx -mabi=n32 > > > > -Sandra
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index 5020208..6ab7dd3 100644 --- a/gcc/config/mips/mips.h +++ b/gcc/config/mips/mips.h @@ -2311,7 +2311,13 @@ enum reg_class #define STACK_GROWS_DOWNWARD 1 -#define FRAME_GROWS_DOWNWARD flag_stack_protect +/* Growing the frame downwards allows us to put spills closest to + the stack pointer which is good as they are likely to be accessed + frequently. We can also arrange for normal stack usage to place + scalars last so that they too are close to the stack pointer. */ +#define FRAME_GROWS_DOWNWARD ((TARGET_MIPS16 \ + && TARGET_FRAME_GROWS_DOWNWARDS) \ + || flag_stack_protect) /* Size of the area allocated in the frame to save the GP. */ diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt index 3b92ef5..53feb23 100644 --- a/gcc/config/mips/mips.opt +++ b/gcc/config/mips/mips.opt @@ -447,3 +447,7 @@ Enum(mips_cb_setting) String(always) Value(MIPS_CB_ALWAYS) minline-intermix Target Report Var(TARGET_INLINE_INTERMIX) Allow inlining even if the compression flags differ between caller and callee. + +mgrow-frame-downwards +Target Report Var(TARGET_FRAME_GROWS_DOWNWARDS) Init(1) +Change the behaviour to grow the frame downwards for MIPS16. diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 2f6195e..6e5d620 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -838,6 +838,7 @@ Objective-C and Objective-C++ Dialects}. -minterlink-compressed -mno-interlink-compressed @gol -minterlink-mips16 -mno-interlink-mips16 @gol -minline-intermix -mno-inline-intermix @gol +-mgrow-frame-downwards -mno-grow-frame-downwards @gol -mabi=@var{abi} -mabicalls -mno-abicalls @gol -mshared -mno-shared -mplt -mno-plt -mxgot -mno-xgot @gol -mgp32 -mgp64 -mfp32 -mfpxx -mfp64 -mhard-float -msoft-float @gol @@ -17929,6 +17930,18 @@ vice-versa. When using this option it is necessary to protect functions that cannot be compiled as MIPS16 with a @code{noinline} attribute to ensure they are not inlined into a MIPS16 function. +@item -mgrow-frame-downwards +@itemx -mno-grow-frame-downwards +@opindex mgrow-frame-downwards +Grow the local frame down (up) for MIPS16. + +Growing the frame downwards allows us to get spill slots created at the lowest +address rather than the highest address in a local frame. The benefit of this +is smaller code size as accessing spill splots closer to the stack pointer +can be done using using 16-bit instructions. + +The option is enabled by default (to grow frame downwards) for MIPS16. + @item -mabi=32 @itemx -mabi=o64 @itemx -mabi=n32