This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [COMMITTED][AArch64] Fix frame tests


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]