diff mbox series

[COMMITTED,AArch64] Fix frame tests

Message ID DB6PR0801MB2053190F6A7C547DB3DBC2E8832E0@DB6PR0801MB2053.eurprd08.prod.outlook.com
State New
Headers show
Series [COMMITTED,AArch64] Fix frame tests | expand

Commit Message

Wilco Dijkstra Nov. 16, 2017, 11:34 a.m. UTC 
Improve the AArch64 frame tests - add -f(no-)omit-frame-pointer,
update checks and add missing tests.  As a result all tests now
pass.

Committed as obvious.

ChangeLog:
2017-11-16  Wilco Dijkstra  <wdijkstr@arm.com>

	* gcc.target/aarch64/lr_free_2.c: Fix 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.
--

Comments

James Greenhalgh Nov. 17, 2017, 10:02 p.m. UTC | #1 
On Thu, Nov 16, 2017 at 11:34:46AM +0000, Wilco Dijkstra wrote:
> Improve the AArch64 frame tests - add -f(no-)omit-frame-pointer,
> update checks and add missing tests.  As a result all tests now
> pass.
> 
> Committed as obvious.

Some of these are far from obvious... Even if they were obvious to you I'd
have appreciated a short description of each change.

For example...

> diff --git a/gcc/testsuite/gcc.target/aarch64/lr_free_2.c b/gcc/testsuite/gcc.target/aarch64/lr_free_2.c
> index e2b9490fab1a27755d239ad6802325a619f73db3..5d9500f4fb144bdae5d0199f0b0a218deb504176 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-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-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" } */
>  
>  extern void abort ();

This test failing and now requiring -fno-omit-frame-pointer implies we have
changed the default of when we omit leaf frame pointers. We may have done
that deliberately, but the testcase change isn't obvious on its own.


> diff --git a/gcc/testsuite/gcc.target/aarch64/spill_1.c b/gcc/testsuite/gcc.target/aarch64/spill_1.c
> index 847425895d456e4433b0d15556d60a66a8f8f70c..c9528cb21daaefcdd5f1218ee13edf40ee44bd99 100644
> --- a/gcc/testsuite/gcc.target/aarch64/spill_1.c
> +++ b/gcc/testsuite/gcc.target/aarch64/spill_1.c
> @@ -14,5 +14,3 @@ 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} } } */

This change doesn't seem to be needed at all?

And so on...

Even if there were broad classes of changes you made to the tests a
justification would be helpful.

The "obvious" rule needs to only apply when the patch would be obvious to
most of the community. I haven't reverted the patch (this time), but I would
appreciate a case-by-case explanation of the patch, if nothing else so we
can understandcyour motives here a few years in the future when we next change
these tests.

Thanks,
James
Ramana Radhakrishnan Nov. 17, 2017, 10:15 p.m. UTC | #2 
Why do we need fno-omit-frame-pointer on aarch64 ?


Ramana


From: James Greenhalgh
Sent: Friday, 17 November, 22:02
Subject: Re: [COMMITTED][AArch64] Fix frame tests
To: Wilco Dijkstra
Cc: GCC Patches, nd, Richard Earnshaw, Marcus Shawcroft, Ramana Radhakrishnan


On Thu, Nov 16, 2017 at 11:34:46AM +0000, Wilco Dijkstra wrote: > Improve the AArch64 frame tests - add -f(no-)omit-frame-pointer, > update checks and add missing tests. As a result all tests now > pass. > > Committed as obvious. Some of these are far from obvious... Even if they were obvious to you I'd have appreciated a short description of each change. For example... > diff --git a/gcc/testsuite/gcc.target/aarch64/lr_free_2.c b/gcc/testsuite/gcc.target/aarch64/lr_free_2.c > index e2b9490fab1a27755d239ad6802325a619f73db3..5d9500f4fb144bdae5d0199f0b0a218deb504176 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-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-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" } */ > > extern void abort (); This test failing and now requiring -fno-omit-frame-pointer implies we have changed the default of when we omit leaf frame pointers. We may have done that deliberately, but the testcase change isn't obvious on its own. > diff --git a/gcc/testsuite/gcc.target/aarch64/spill_1.c b/gcc/testsuite/gcc.target/aarch64/spill_1.c > index 847425895d456e4433b0d15556d60a66a8f8f70c..c9528cb21daaefcdd5f1218ee13edf40ee44bd99 100644 > --- a/gcc/testsuite/gcc.target/aarch64/spill_1.c > +++ b/gcc/testsuite/gcc.target/aarch64/spill_1.c > @@ -14,5 +14,3 @@ 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} } } */ This change doesn't seem to be needed at all? And so on... Even if there were broad classes of changes you made to the tests a justification would be helpful. The "obvious" rule needs to only apply when the patch would be obvious to most of the community. I haven't reverted the patch (this time), but I would appreciate a case-by-case explanation of the patch, if nothing else so we can understandcyour motives here a few years in the future when we next change these tests. Thanks, James
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.target/aarch64/lr_free_2.c b/gcc/testsuite/gcc.target/aarch64/lr_free_2.c
index e2b9490fab1a27755d239ad6802325a619f73db3..5d9500f4fb144bdae5d0199f0b0a218deb504176 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-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-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" } */
 
 extern void abort ();
 
diff --git a/gcc/testsuite/gcc.target/aarch64/spill_1.c b/gcc/testsuite/gcc.target/aarch64/spill_1.c
index 847425895d456e4433b0d15556d60a66a8f8f70c..c9528cb21daaefcdd5f1218ee13edf40ee44bd99 100644
--- a/gcc/testsuite/gcc.target/aarch64/spill_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/spill_1.c
@@ -14,5 +14,3 @@  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 f162cc091e0090ece943715ae573d0c11821b19b..67f858260d9156ac00951a21aa57353182188133 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 "-O2 --save-temps" } */
+/* { dg-options "-fno-omit-frame-pointer -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 62761e7ff9b3fd0afc064f9a9b737583261b0610..02e48b4acac0a27a4beca0a3011d8d2c1a408117 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 --save-temps" } */
+/* { dg-options "-O2 -fomit-frame-pointer --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 "ldp\tx29, x30, \\\[sp, \[0-9\]+\\\]" } } */
+/* { dg-final { scan-assembler "ldr\tx30, \\\[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 74b3370fa463b652265e00fff80cc8856524d509..33139363785d6befcb5eb009bc4324df785d32c4 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 "-O2 --save-temps" } */
+/* { dg-options "-fno-omit-frame-pointer -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 78818dec32af95c43b610cab1832ea29041c3b36..acef2bbffc82885b3c05d2c902921461bb389c23 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_14.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_14.c
@@ -4,9 +4,12 @@ 
      * number of callee-save reg >= 2.  */
 
 /* { dg-do run } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -fno-omit-frame-pointer --save-temps" } */
 
 #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 bed6714b4fe529a3b81ad8c5253924aa97bf8806..aebf6d1e43f7cdd70eb927fd365a78f122ccea62 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 "-O2 --save-temps" } */
+/* { dg-options "-fno-omit-frame-pointer -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 f90ea4a1ae880c69d88cf8b38558312510e66c63..b34a0ddd41d650521ca68d96bc79f0645b69caa0 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_3.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_3.c
@@ -6,9 +6,12 @@ 
      * we can't use "str !" to optimize stack adjustment.  */
 
 /* { dg-do run } */
-/* { dg-options "-O2 -fomit-frame-pointer" } */
+/* { dg-options "-O2 -fomit-frame-pointer --save-temps" } */
 
 #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 0624b5b747339b851daff5d265701c241e5c1231..2d7c83a7404bc97a959f534bb2344aa3607a9e22 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_5.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_5.c
@@ -5,9 +5,12 @@ 
      * one subtraction of the whole frame size.  */
 
 /* { dg-do run } */
-/* { dg-options "-O2 -fomit-frame-pointer" } */
+/* { dg-options "-O2 -fomit-frame-pointer --save-temps" } */
 
 #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 0dffbf8ad1706d224b933a9a849f3136797e97a3..a28fbcee9598a52cacc08a40694430ad4d2d7104 100644
--- a/gcc/testsuite/gcc.target/aarch64/test_frame_9.c
+++ b/gcc/testsuite/gcc.target/aarch64/test_frame_9.c
@@ -4,14 +4,15 @@ 
      * total frame size > 512.
        area except outgoing <= 512
      * number of callee-saved reg = 1.
-     * Split stack adjustment into two subtractions.
-       the first subtractions couldn't be optimized
-       into "str !" as it's > 256.  */
+     * Use a single stack adjustment.  */
 
 /* { dg-do run } */
-/* { dg-options "-O2 -fomit-frame-pointer" } */
+/* { dg-options "-O2 -fomit-frame-pointer --save-temps" } */
 
 #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\]+\\\]" } } */