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

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

> -----Original Message-----
> From: Steve Ellcey [mailto:sellcey@imgtec.com]
> Sent: Monday, October 05, 2015 1:16 PM
> To: Moore, Catherine
> Cc: Matthew Fortune; GCC Patches
> Subject: RE: [PATCH, MIPS] Frame header optimization for MIPS O32 ABI
> 
> On Mon, 2015-09-28 at 22:10 +0000, Moore, Catherine wrote:
> 
> > Hi Steve, I'm sorry for the delay in reviewing this patch.
> > Some changes have been committed upstream (see revision #227941) that
> > will require updates to this patch.
> > Please post the update for review.  Other comments are embedded.
> 
> OK, I have updated the comments based on your input and changed the
> code to compile with the ToT GCC after revision @227941.  Here is the new
> patch.
> 

HI Sreve,

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" }

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.

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 } {
>