[Patch AArch64] Turn on frame pointer / partial fix for PR84521

[Patch AArch64] Turn on frame pointer / partial fix for PR84521

[Ramana Radhakrishnan]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

This fixes a GCC-8 regression that we accidentally switched off frame
pointers in the AArch64 backend when changing the defaults in the common
parts of the code. This breaks an ABI decision that was made in GCC at
the dawn of the port with respect to having a frame pointer at all
times.  If we really want to turn this off lets have a discussion around
that separately.

For now turn this back on and I believe this will leave PR84521 latent
again with -fomit-frame-pointer and (hopefully) make the ruby issue go
away. I'm asking Sudi to pick that up.

Bootstrapped and regression tested on AArch64-none-linux-gnu but I see
one regression in gcc.c-torture/execute/960419-2.c which needs to be
looked at next (PR84528, thanks Kyrill).

Ok to put in and then look at PR84528 ?

gcc/ChangeLog:

2018-02-23  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>

        PR target/84521
	* common/config/aarch64/aarch64-common.c
(aarch_option_optimization_table[]): Switch
off fomit-frame-pointer

gcc/testsuite/ChangeLog:

2018-02-23  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>

	* gcc.target/aarch64/lr_free_2.c: Adjust test.
	* gcc.target/aarch64/spill_1.c: Likewise.
	* gcc.target/aarch64/test_frame_11.c: Likewise.
	* gcc.target/aarch64/test_frame_12.c: Likewise.
	* gcc.target/aarch64/test_frame_13.c: Likewise.
	* gcc.target/aarch64/test_frame_14.c: Likewise.
	* gcc.target/aarch64/test_frame_15.c: Likewise.
	* gcc.target/aarch64/test_frame_3.c: Likewise.
	* gcc.target/aarch64/test_frame_5.c: Likewise.
	* gcc.target/aarch64/test_frame_9.c: Likewise.

[-- Attachment #2: fixup-frame-pointer.txt --]
[-- Type: text/plain, Size: 7784 bytes --]

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 71d395398c7..7fd93050037 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -47,6 +47,8 @@ static const struct default_options aarch_option_optimization_table[] =
   {
     /* Enable section anchors by default at -O1 or higher.  */
     { OPT_LEVELS_1_PLUS, OPT_fsection_anchors, NULL, 1 },
+    /* Disable fomit-frame-pointer by default.  */
+    { OPT_LEVELS_ALL, OPT_fomit_frame_pointer, NULL, 0 },
     /* Enable -fsched-pressure by default when optimizing.  */
     { OPT_LEVELS_1_PLUS, OPT_fsched_pressure, NULL, 1 },
     /* Enable redundant extension instructions removal at -O2 and higher.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/lr_free_2.c b/gcc/testsuite/gcc.target/aarch64/lr_free_2.c
index 5d9500f4fb1..e2b9490fab1 100644
--- a/gcc/testsuite/gcc.target/aarch64/lr_free_2.c
+++ b/gcc/testsuite/gcc.target/aarch64/lr_free_2.c
@@ -1,5 +1,5 @@
 /* { dg-do run } */
-/* { dg-options "-fno-omit-frame-pointer -fno-inline -O2 -ffixed-x2 -ffixed-x3 -ffixed-x4 -ffixed-x5 -ffixed-x6 -ffixed-x7 -ffixed-x8 -ffixed-x9 -ffixed-x10 -ffixed-x11 -ffixed-x12 -ffixed-x13 -ffixed-x14 -ffixed-x15 -ffixed-x16 -ffixed-x17 -ffixed-x18 -ffixed-x19 -ffixed-x20 -ffixed-x21 -ffixed-x22 -ffixed-x23 -ffixed-x24 -ffixed-x25 -ffixed-x26 -ffixed-x27 -ffixed-x28 --save-temps -mgeneral-regs-only -fno-ipa-cp -fdump-rtl-ira" } */
+/* { dg-options "-fno-inline -O2 -ffixed-x2 -ffixed-x3 -ffixed-x4 -ffixed-x5 -ffixed-x6 -ffixed-x7 -ffixed-x8 -ffixed-x9 -ffixed-x10 -ffixed-x11 -ffixed-x12 -ffixed-x13 -ffixed-x14 -ffixed-x15 -ffixed-x16 -ffixed-x17 -ffixed-x18 -ffixed-x19 -ffixed-x20 -ffixed-x21 -ffixed-x22 -ffixed-x23 -ffixed-x24 -ffixed-x25 -ffixed-x26 -ffixed-x27 -ffixed-x28 --save-temps -mgeneral-regs-only -fno-ipa-cp -fdump-rtl-ira" } */
 
 extern void abort ();
 
diff --git a/gcc/testsuite/gcc.target/aarch64/spill_1.c b/gcc/testsuite/gcc.target/aarch64/spill_1.c
index c9528cb21da..847425895d4 100644
--- a/gcc/testsuite/gcc.target/aarch64/spill_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/spill_1.c
@@ -14,3 +14,5 @@ foo (void)
 }
 
 /* { dg-final { scan-assembler-times {\tmovi\tv[0-9]+\.4s,} 2 } } */
+/* { dg-final { scan-assembler-not {\tldr\t} } } */
+/* { dg-final { scan-assembler-not {\tstr\t} } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/test_frame_11.c b/gcc/testsuite/gcc.target/aarch64/test_frame_11.c
index 67f858260d9..f162cc091e0 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_11.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_11.c
@@ -5,7 +5,7 @@
      * optimized code should use "stp !" for stack adjustment.  */
 
 /* { dg-do run } */
-/* { dg-options "-fno-omit-frame-pointer -O2 --save-temps" } */
+/* { dg-options "-O2 --save-temps" } */
 
 #include "test_frame_common.h"
 
diff --git a/gcc/testsuite/gcc.target/aarch64/test_frame_12.c b/gcc/testsuite/gcc.target/aarch64/test_frame_12.c
index 02e48b4acac..62761e7ff9b 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_12.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_12.c
@@ -4,7 +4,7 @@
      * number of callee-save reg >= 2.  */
 
 /* { dg-do run } */
-/* { dg-options "-O2 -fomit-frame-pointer --save-temps" } */
+/* { dg-options "-O2 --save-temps" } */
 
 #include "test_frame_common.h"
 
@@ -14,5 +14,5 @@ t_frame_run (test12)
 /* { dg-final { scan-assembler-times "sub\tsp, sp, #\[0-9\]+" 1 } } */
 
 /* Check epilogue using no write-back.  */
-/* { dg-final { scan-assembler "ldr\tx30, \\\[sp, \[0-9\]+\\\]" } } */
+/* { dg-final { scan-assembler "ldp\tx29, x30, \\\[sp, \[0-9\]+\\\]" } } */
 
diff --git a/gcc/testsuite/gcc.target/aarch64/test_frame_13.c b/gcc/testsuite/gcc.target/aarch64/test_frame_13.c
index 33139363785..74b3370fa46 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_13.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_13.c
@@ -5,7 +5,7 @@
      * Use a single stack adjustment, no writeback.  */
 
 /* { dg-do run } */
-/* { dg-options "-fno-omit-frame-pointer -O2 --save-temps" } */
+/* { dg-options "-O2 --save-temps" } */
 
 #include "test_frame_common.h"
 
diff --git a/gcc/testsuite/gcc.target/aarch64/test_frame_14.c b/gcc/testsuite/gcc.target/aarch64/test_frame_14.c
index 95e2bf301d7..78818dec32a 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_14.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_14.c
@@ -4,12 +4,9 @@
      * number of callee-save reg >= 2.  */
 
 /* { dg-do run } */
-/* { dg-options "-O2 -fno-omit-frame-pointer --save-temps" } */
+/* { dg-options "-O2" } */
 
 #include "test_frame_common.h"
 
 t_frame_pattern_outgoing (test14, 700, , 8, a[8])
 t_frame_run (test14)
-
-/* { dg-final { scan-assembler-times "sub\tsp, sp, #\[0-9\]+" 1 } } */
-/* { dg-final { scan-assembler-times "stp\tx29, x30, \\\[sp, \[0-9\]+\\\]" 1 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/test_frame_15.c b/gcc/testsuite/gcc.target/aarch64/test_frame_15.c
index aebf6d1e43f..bed6714b4fe 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_15.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_15.c
@@ -6,7 +6,7 @@
      * Use a single stack adjustment, no writeback.  */
 
 /* { dg-do run } */
-/* { dg-options "-fno-omit-frame-pointer -O2 --save-temps" } */
+/* { dg-options "-O2 --save-temps" } */
 
 #include "test_frame_common.h"
 
diff --git a/gcc/testsuite/gcc.target/aarch64/test_frame_3.c b/gcc/testsuite/gcc.target/aarch64/test_frame_3.c
index b34a0ddd41d..f90ea4a1ae8 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_3.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_3.c
@@ -6,12 +6,9 @@
      * we can't use "str !" to optimize stack adjustment.  */
 
 /* { dg-do run } */
-/* { dg-options "-O2 -fomit-frame-pointer --save-temps" } */
+/* { dg-options "-O2 -fomit-frame-pointer" } */
 
 #include "test_frame_common.h"
 
 t_frame_pattern (test3, 400, )
 t_frame_run (test3)
-
-/* { dg-final { scan-assembler-times "sub\tsp, sp, #\[0-9\]+" 1 } } */
-/* { dg-final { scan-assembler "str\tx30, \\\[sp\\\]" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/test_frame_5.c b/gcc/testsuite/gcc.target/aarch64/test_frame_5.c
index 2d7c83a7404..0624b5b7473 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_5.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_5.c
@@ -5,12 +5,9 @@
      * one subtraction of the whole frame size.  */
 
 /* { dg-do run } */
-/* { dg-options "-O2 -fomit-frame-pointer --save-temps" } */
+/* { dg-options "-O2 -fomit-frame-pointer" } */
 
 #include "test_frame_common.h"
 
 t_frame_pattern_outgoing (test5, 300, "x19", 8, a[8])
 t_frame_run (test5)
-
-/* { dg-final { scan-assembler-times "sub\tsp, sp, #\[0-9\]+" 1 } } */
-/* { dg-final { scan-assembler "stp\tx\[0-9\]+, x30, \\\[sp, \[0-9\]+\\\]" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/test_frame_9.c b/gcc/testsuite/gcc.target/aarch64/test_frame_9.c
index a28fbcee959..0dffbf8ad17 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_9.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_9.c
@@ -4,15 +4,14 @@
      * total frame size > 512.
        area except outgoing <= 512
      * number of callee-saved reg = 1.
-     * Use a single stack adjustment.  */
+     * Split stack adjustment into two subtractions.
+       the first subtractions couldn't be optimized
+       into "str !" as it's > 256.  */
 
 /* { dg-do run } */
-/* { dg-options "-O2 -fomit-frame-pointer --save-temps" } */
+/* { dg-options "-O2 -fomit-frame-pointer" } */
 
 #include "test_frame_common.h"
 
 t_frame_pattern_outgoing (test9, 480, , 24, a[8], a[9], a[10])
 t_frame_run (test9)
-
-/* { dg-final { scan-assembler-times "sub\tsp, sp, #\[0-9\]+" 1 } } */
-/* { dg-final { scan-assembler "str\tx30, \\\[sp, \[0-9\]+\\\]" } } */

Re: [Patch AArch64] Turn on frame pointer / partial fix for PR84521

[James Greenhalgh]

On Fri, Feb 23, 2018 at 11:32:57AM +0000, Ramana Radhakrishnan wrote:
> This fixes a GCC-8 regression that we accidentally switched off frame
> pointers in the AArch64 backend when changing the defaults in the common
> parts of the code. This breaks an ABI decision that was made in GCC at
> the dawn of the port with respect to having a frame pointer at all
> times.  If we really want to turn this off lets have a discussion around
> that separately.
> 
> For now turn this back on and I believe this will leave PR84521 latent
> again with -fomit-frame-pointer and (hopefully) make the ruby issue go
> away. I'm asking Sudi to pick that up.
> 
> Bootstrapped and regression tested on AArch64-none-linux-gnu but I see
> one regression in gcc.c-torture/execute/960419-2.c which needs to be
> looked at next (PR84528, thanks Kyrill).
> 
> Ok to put in and then look at PR84528 ?

OK.

Are your testsuite chanmges just the revert of r254814, your ChangeLog
should reflect that.

Thanks,
James

> 
> gcc/ChangeLog:
> 
> 2018-02-23  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
> 
>         PR target/84521
> 	* common/config/aarch64/aarch64-common.c
> (aarch_option_optimization_table[]): Switch
> off fomit-frame-pointer
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-02-23  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
> 
> 	* gcc.target/aarch64/lr_free_2.c: Adjust test.
> 	* gcc.target/aarch64/spill_1.c: Likewise.
> 	* gcc.target/aarch64/test_frame_11.c: Likewise.
> 	* gcc.target/aarch64/test_frame_12.c: Likewise.
> 	* gcc.target/aarch64/test_frame_13.c: Likewise.
> 	* gcc.target/aarch64/test_frame_14.c: Likewise.
> 	* gcc.target/aarch64/test_frame_15.c: Likewise.
> 	* gcc.target/aarch64/test_frame_3.c: Likewise.
> 	* gcc.target/aarch64/test_frame_5.c: Likewise.
> 	* gcc.target/aarch64/test_frame_9.c: Likewise.

Re: [Patch AArch64] Turn on frame pointer / partial fix for PR84521

[Jakub Jelinek]

On Fri, Feb 23, 2018 at 11:32:57AM +0000, Ramana Radhakrishnan wrote:
> This fixes a GCC-8 regression that we accidentally switched off frame
> pointers in the AArch64 backend when changing the defaults in the common
> parts of the code. This breaks an ABI decision that was made in GCC at
> the dawn of the port with respect to having a frame pointer at all
> times.  If we really want to turn this off lets have a discussion around
> that separately.
> 
> For now turn this back on and I believe this will leave PR84521 latent
> again with -fomit-frame-pointer and (hopefully) make the ruby issue go
> away. I'm asking Sudi to pick that up.

I don't think it will change anything on PR84521 testcase with
-fomit-frame-pointer, only make it pass without either of
-f{,no-}omit-frame-pointer where it previously failed.  It will still fail
with -fomit-frame-pointer and didn't use to fail and won't fail with
explicit -fno-omit-frame-pointer.

The general decision if frame pointers should be used by default or not
depends on performance (so the patch to change it in generic code really
seems to be weird); if -fomit-frame-pointer is faster than
-fno-omit-frame-pointer in general, then it is a sane default.  Code like
backtrace can't assume frame pointers are always available anyway, because
some frames could be compiled with -fomit-frame-pointer and generally there
is unwind info that can be used for backtrace like purposes (though,
for some strange reason aarch64 doesn't enable -fasynchronous-unwind-tables
by default).

	Jakub

Re: [Patch AArch64] Turn on frame pointer / partial fix for PR84521

[David Malcolm]

On Fri, 2018-02-23 at 11:32 +0000, Ramana Radhakrishnan wrote:
> This fixes a GCC-8 regression that we accidentally switched off frame
> pointers in the AArch64 backend when changing the defaults in the
> common
> parts of the code. This breaks an ABI decision that was made in GCC
> at
> the dawn of the port with respect to having a frame pointer at all
> times.  If we really want to turn this off lets have a discussion
> around
> that separately.
> 
> For now turn this back on and I believe this will leave PR84521
> latent
> again with -fomit-frame-pointer and (hopefully) make the ruby issue
> go
> away. I'm asking Sudi to pick that up.

FWIW I was able to successfully build ruby on aarch64 with this patch
(on gcc116).

[...snip...]

Re: [Patch AArch64] Turn on frame pointer / partial fix for PR84521

[Richard Sandiford]

Ramana Radhakrishnan <ramana.radhakrishnan@foss.arm.com> writes:
> This fixes a GCC-8 regression that we accidentally switched off frame
> pointers in the AArch64 backend when changing the defaults in the common
> parts of the code. This breaks an ABI decision that was made in GCC at
> the dawn of the port with respect to having a frame pointer at all
> times.  If we really want to turn this off lets have a discussion around
> that separately.
>
> For now turn this back on and I believe this will leave PR84521 latent
> again with -fomit-frame-pointer and (hopefully) make the ruby issue go
> away. I'm asking Sudi to pick that up.
>
> Bootstrapped and regression tested on AArch64-none-linux-gnu but I see
> one regression in gcc.c-torture/execute/960419-2.c which needs to be
> looked at next (PR84528, thanks Kyrill).
>
> Ok to put in and then look at PR84528 ?
>
> gcc/ChangeLog:
>
> 2018-02-23  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
>
>         PR target/84521
> 	* common/config/aarch64/aarch64-common.c
> (aarch_option_optimization_table[]): Switch
> off fomit-frame-pointer

If we're doing this, we might also want to change the default for
-momit-leaf-frame-pointer.  As things stand we'll create a frame
for things like:

  void f (void) { volatile int x = 1; }

but not use a frame pointer for it.

Thanks,
Richard