RE: [PATCH, MIPS] Frame header optimization for MIPS O32 ABI

["Moore, Catherine" <Catherine_Moore at mentor dot com>]

> -----Original Message-----
> From: Matthew Fortune [mailto:Matthew.Fortune@imgtec.com]
> Sent: Tuesday, October 06, 2015 4:39 AM
> To: Moore, Catherine; Steve Ellcey
> Cc: GCC Patches
> Subject: RE: [PATCH, MIPS] Frame header optimization for MIPS O32 ABI
> 
> Moore, Catherine <Catherine_Moore@mentor.com> writes:
> > The patch itself looks good, but the tests that you added need a little more
> work.
> >
> > I tested with the mips-sde-elf-lite configuration and I'm seeing
> > failures for many options.  The main failure mode seems to be that the
> > stack is incremented by 24 instead of 32.
> > I tried this change in frame-header-1.c and frame-header-3.c:
> >
> > /* { dg-final { scan-assembler-not "\taddiu\t\\\$sp,\\\$sp,-8" } }
> >
> > instead of:
> >
> > /* { dg-final { scan-assembler "\taddiu\t\\\$sp,\\\$sp,-32" }
> 
> I'd quite like to be specific about the frame layout we expect as the testcase
> is so simple that I think it should be predictable over time. Did you happen to
> see a pattern to the failure? i.e. Could it be non-o32 ABIs? I'm not a fan of
> scan-assembler-nots in general as it is so easy to change exact output text
> and never match them anyway even if the offending instruction is
> generated.
There was not a pattern to the failure and I witnessed it for o32 ABIs.
> 
> > There are still errors in frame-header-2.c when compiling with -mips16
> > and -mabi=64 (this one uses a daddiu).
> > Also, the tests fail for -flto variants.
> 
> Let's just add NOMIPS16 (I think that is the macro) to the functions and lock
> the tests down to -mabi=32 which is the only ABI they are valid for anyway.
> 
> Matthew
> 
> > Would you please fix this up and resubmit?
> >
> > Thanks,
> > Catherine
> >
> > >
> > >
> > > diff --git a/gcc/testsuite/gcc.target/mips/frame-header-1.c
> > > b/gcc/testsuite/gcc.target/mips/frame-header-1.c
> > > new file mode 100644
> > > index 0000000..8495e0f
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/mips/frame-header-1.c
> > > @@ -0,0 +1,21 @@
> > > +/* Verify that we do not optimize away the frame header in foo when
> using
> > > +   -mno-frame-header-opt by checking the stack pointer increment done
> in
> > > +   that function.  Without the optimization foo should increment the
> stack
> > > +   by 32 bytes, with the optimization it would only be 8 bytes.  */
> > > +
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-mno-frame-header-opt" } */
> > > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
> > > +/* { dg-final { scan-assembler "\taddiu\t\\\$sp,\\\$sp,-32" } } */
> > > +
> > > +void __attribute__((noinline))
> > > +bar (int* a)
> > > +{
> > > +  *a = 1;
> > > +}
> > > +
> > > +void
> > > +foo (int a)
> > > +{
> > > +  bar (&a);
> > > +}
> > > diff --git a/gcc/testsuite/gcc.target/mips/frame-header-2.c
> > > b/gcc/testsuite/gcc.target/mips/frame-header-2.c
> > > new file mode 100644
> > > index 0000000..37ea2d1
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/mips/frame-header-2.c
> > > @@ -0,0 +1,21 @@
> > > +/* Verify that we do optimize away the frame header in foo when using
> > > +   -mframe-header-opt by checking the stack pointer increment done in
> > > +   that function.  Without the optimization foo should increment the
> > > +   stack by 32 bytes, with the optimization it would only be 8 bytes.
> > > +*/
> > > +
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-mframe-header-opt" } */
> > > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
> > > +/* { dg-final { scan-assembler "\taddiu\t\\\$sp,\\\$sp,-8" } } */
> > > +
> > > +void __attribute__((noinline))
> > > +bar (int* a)
> > > +{
> > > +  *a = 1;
> > > +}
> > > +
> > > +void
> > > +foo (int a)
> > > +{
> > > +  bar (&a);
> > > +}
> > > diff --git a/gcc/testsuite/gcc.target/mips/frame-header-3.c
> > > b/gcc/testsuite/gcc.target/mips/frame-header-3.c
> > > new file mode 100644
> > > index 0000000..1cb1547
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/mips/frame-header-3.c
> > > @@ -0,0 +1,22 @@
> > > +/* Verify that we do not optimize away the frame header in foo when
> using
> > > +   -mframe-header-opt but are calling a weak function that may be
> > > overridden
> > > +   by a different function that does need the frame header.  Without the
> > > +   optimization foo should increment the stack by 32 bytes, with the
> > > +   optimization it would only be 8 bytes.  */
> > > +
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-mframe-header-opt" } */
> > > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
> > > +/* { dg-final { scan-assembler "\taddiu\t\\\$sp,\\\$sp,-32" } } */
> > > +
> > > +void __attribute__((noinline, weak)) bar (int* a) {
> > > +  *a = 1;
> > > +}
> > > +
> > > +void
> > > +foo (int a)
> > > +{
> > > +  bar (&a);
> > > +}
> > > diff --git a/gcc/testsuite/gcc.target/mips/mips.exp
> > > b/gcc/testsuite/gcc.target/mips/mips.exp
> > > index 42e7fff..0f2d6a2 100644
> > > --- a/gcc/testsuite/gcc.target/mips/mips.exp
> > > +++ b/gcc/testsuite/gcc.target/mips/mips.exp
> > > @@ -256,6 +256,7 @@ set mips_option_groups {
> > >      maddps "HAS_MADDPS"
> > >      lsa "(|!)HAS_LSA"
> > >      section_start "-Wl,--section-start=.*"
> > > +    frame-header "-mframe-header-opt|-mno-frame-header-opt"
> > >  }
> > >
> > >  for { set option 0 } { $option < 32 } { incr option } {
> > >