RE: [PATCH][MIPS] Add -mgrow-frame-downwards option

[Matthew Fortune <Matthew dot Fortune at imgtec dot com>]

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