public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH][PR target/81535] Fix tests on Power
@ 2017-07-28  4:42 Yury Gribov
  2017-08-04 20:35 ` [PATCH][PING][PR " Yury Gribov
  2017-08-07 23:33 ` [PATCH][PR " Segher Boessenkool
  0 siblings, 2 replies; 4+ messages in thread
From: Yury Gribov @ 2017-07-28  4:42 UTC (permalink / raw)
  To: gcc-patches; +Cc: seurer, wschmidt, meissner, marxin

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

Hi all,

This patch fixes issues reported in 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81535

I removed call to g in pr79439.c because gcc was duplicating the basic 
block with call depending on compiler flags (so scan-assembler-times 
pattern wasn't reliable anymore).  I also added alias to prevent 
inlining introduced by recent PR56727 patch.

I added Power-specific pattern in pr56727-2.c testcase and disabled 
testing on Power in pr56727-1.c.

Tested on powerpc64-unknown-linux-gnu.  Ok for trunk?

-Y

[-- Attachment #2: pr81535-1.patch --]
[-- Type: text/plain, Size: 2438 bytes --]

2017-07-28  Yury Gribov  <tetra2005@gmail.com>

	PR target/81535
	* gcc.dg/pr56727-1.c: Do not check output on Power.
	* gcc.dg/pr56727-2.c: Fix pattern for Power.
	* gcc.target/powerpc/pr79439.c: Prevent inlining.

diff -rupN gcc/gcc/testsuite/gcc.dg/pr56727-1.c gcc-81535/gcc/testsuite/gcc.dg/pr56727-1.c
--- gcc/gcc/testsuite/gcc.dg/pr56727-1.c	2017-07-28 02:39:54.770046466 +0000
+++ gcc-81535/gcc/testsuite/gcc.dg/pr56727-1.c	2017-07-28 04:25:04.805648587 +0000
@@ -1,6 +1,6 @@
 /* { dg-do compile { target fpic } } */
 /* { dg-options "-O2 -fPIC" } */
-/* { dg-final { scan-assembler-not "@(PLT|plt)" { target i?86-*-* x86_64-*-* powerpc*-*-* } } } */
+/* { dg-final { scan-assembler-not "@(PLT|plt)" { target i?86-*-* x86_64-*-* } } } */
 
 #define define_func(type) \
   void f_ ## type (type b) { f_ ## type (0); } \
diff -rupN gcc/gcc/testsuite/gcc.dg/pr56727-2.c gcc-81535/gcc/testsuite/gcc.dg/pr56727-2.c
--- gcc/gcc/testsuite/gcc.dg/pr56727-2.c	2017-07-28 02:39:54.770046466 +0000
+++ gcc-81535/gcc/testsuite/gcc.dg/pr56727-2.c	2017-07-28 04:21:19.195215187 +0000
@@ -1,10 +1,10 @@
 /* { dg-do compile { target fpic } } */
 /* { dg-options "-O2 -fPIC" } */
-/* { dg-final { scan-assembler "@(PLT|plt)" { target i?86-*-* x86_64-*-* powerpc*-*-linux* } } } */
 
 __attribute__((noinline, noclone))
 void f (short b)
 {
+  __builtin_setjmp (0);  /* Prevent tailcall */
   f (0);
 }
 
@@ -14,3 +14,5 @@ void h ()
 {
   g (0);
 }
+/* { dg-final { scan-assembler "@(PLT|plt)" { target i?86-*-* x86_64-*-* } } } */
+/* { dg-final { scan-assembler "bl f\n\[ \t\]*nop" { target powerpc*-*-linux* } } } */
diff -rupN gcc/gcc/testsuite/gcc.target/powerpc/pr79439.c gcc-81535/gcc/testsuite/gcc.target/powerpc/pr79439.c
--- gcc/gcc/testsuite/gcc.target/powerpc/pr79439.c	2017-07-28 02:39:55.750048426 +0000
+++ gcc-81535/gcc/testsuite/gcc.target/powerpc/pr79439.c	2017-07-28 04:13:47.834177237 +0000
@@ -8,22 +8,17 @@
 
 int f (void);
 
-void
-g (void)
-{
-}
-
 int
 rec (int a)
 {
   int ret = 0;
   if (a > 10 && f ())
     ret += rec (a - 1);
-  g ();
   return a + ret;
 }
 
+void rec_alias (short) __attribute__ ((alias ("rec")));
+
 /* { dg-final { scan-assembler-times {\mbl f\M}   1 } } */
-/* { dg-final { scan-assembler-times {\mbl g\M}   1 } } */
 /* { dg-final { scan-assembler-times {\mbl rec\M} 1 } } */
-/* { dg-final { scan-assembler-times {\mnop\M}    3 } } */
+/* { dg-final { scan-assembler-times {\mnop\M}    2 } } */

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH][PING][PR target/81535] Fix tests on Power
  2017-07-28  4:42 [PATCH][PR target/81535] Fix tests on Power Yury Gribov
@ 2017-08-04 20:35 ` Yury Gribov
  2017-08-07 23:33 ` [PATCH][PR " Segher Boessenkool
  1 sibling, 0 replies; 4+ messages in thread
From: Yury Gribov @ 2017-08-04 20:35 UTC (permalink / raw)
  To: GCC Patches; +Cc: seurer, wschmidt, meissner, marxin

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

Hi all,

This patch fixes issues reported in 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81535

I removed call to g in pr79439.c because gcc was duplicating the basic 
block with call depending on compiler flags (so scan-assembler-times 
pattern wasn't reliable anymore).  I also added alias to prevent 
inlining introduced by recent PR56727 patch.

I added Power-specific pattern in pr56727-2.c testcase and disabled 
testing on Power in pr56727-1.c.

Tested on powerpc64-unknown-linux-gnu.  Ok for trunk?

-Y


[-- Attachment #2: pr81535-1.patch --]
[-- Type: text/plain, Size: 2439 bytes --]

2017-07-28  Yury Gribov  <tetra2005@gmail.com>

	PR target/81535
	* gcc.dg/pr56727-1.c: Do not check output on Power.
	* gcc.dg/pr56727-2.c: Fix pattern for Power.
	* gcc.target/powerpc/pr79439.c: Prevent inlining.

diff -rupN gcc/gcc/testsuite/gcc.dg/pr56727-1.c gcc-81535/gcc/testsuite/gcc.dg/pr56727-1.c
--- gcc/gcc/testsuite/gcc.dg/pr56727-1.c	2017-07-28 02:39:54.770046466 +0000
+++ gcc-81535/gcc/testsuite/gcc.dg/pr56727-1.c	2017-07-28 04:25:04.805648587 +0000
@@ -1,6 +1,6 @@
 /* { dg-do compile { target fpic } } */
 /* { dg-options "-O2 -fPIC" } */
-/* { dg-final { scan-assembler-not "@(PLT|plt)" { target i?86-*-* x86_64-*-* powerpc*-*-* } } } */
+/* { dg-final { scan-assembler-not "@(PLT|plt)" { target i?86-*-* x86_64-*-* } } } */
 
 #define define_func(type) \
   void f_ ## type (type b) { f_ ## type (0); } \
diff -rupN gcc/gcc/testsuite/gcc.dg/pr56727-2.c gcc-81535/gcc/testsuite/gcc.dg/pr56727-2.c
--- gcc/gcc/testsuite/gcc.dg/pr56727-2.c	2017-07-28 02:39:54.770046466 +0000
+++ gcc-81535/gcc/testsuite/gcc.dg/pr56727-2.c	2017-07-28 04:21:19.195215187 +0000
@@ -1,10 +1,10 @@
 /* { dg-do compile { target fpic } } */
 /* { dg-options "-O2 -fPIC" } */
-/* { dg-final { scan-assembler "@(PLT|plt)" { target i?86-*-* x86_64-*-* powerpc*-*-linux* } } } */
 
 __attribute__((noinline, noclone))
 void f (short b)
 {
+  __builtin_setjmp (0);  /* Prevent tailcall */
   f (0);
 }
 
@@ -14,3 +14,5 @@ void h ()
 {
   g (0);
 }
+/* { dg-final { scan-assembler "@(PLT|plt)" { target i?86-*-* x86_64-*-* } } } */
+/* { dg-final { scan-assembler "bl f\n\[ \t\]*nop" { target powerpc*-*-linux* } } } */
diff -rupN gcc/gcc/testsuite/gcc.target/powerpc/pr79439.c gcc-81535/gcc/testsuite/gcc.target/powerpc/pr79439.c
--- gcc/gcc/testsuite/gcc.target/powerpc/pr79439.c	2017-07-28 02:39:55.750048426 +0000
+++ gcc-81535/gcc/testsuite/gcc.target/powerpc/pr79439.c	2017-07-28 04:13:47.834177237 +0000
@@ -8,22 +8,17 @@
 
 int f (void);
 
-void
-g (void)
-{
-}
-
 int
 rec (int a)
 {
   int ret = 0;
   if (a > 10 && f ())
     ret += rec (a - 1);
-  g ();
   return a + ret;
 }
 
+void rec_alias (short) __attribute__ ((alias ("rec")));
+
 /* { dg-final { scan-assembler-times {\mbl f\M}   1 } } */
-/* { dg-final { scan-assembler-times {\mbl g\M}   1 } } */
 /* { dg-final { scan-assembler-times {\mbl rec\M} 1 } } */
-/* { dg-final { scan-assembler-times {\mnop\M}    3 } } */
+/* { dg-final { scan-assembler-times {\mnop\M}    2 } } */


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH][PR target/81535] Fix tests on Power
  2017-07-28  4:42 [PATCH][PR target/81535] Fix tests on Power Yury Gribov
  2017-08-04 20:35 ` [PATCH][PING][PR " Yury Gribov
@ 2017-08-07 23:33 ` Segher Boessenkool
  2017-11-25 10:00   ` Yury Gribov
  1 sibling, 1 reply; 4+ messages in thread
From: Segher Boessenkool @ 2017-08-07 23:33 UTC (permalink / raw)
  To: Yury Gribov; +Cc: gcc-patches, seurer, wschmidt, meissner, marxin

Hi Yuri,

Sorry I missed this.  Please cc: me to prevent that from happening.

On Fri, Jul 28, 2017 at 05:42:00AM +0100, Yury Gribov wrote:
> This patch fixes issues reported in 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81535
> 
> I removed call to g in pr79439.c because gcc was duplicating the basic 
> block with call depending on compiler flags (so scan-assembler-times 
> pattern wasn't reliable anymore).  I also added alias to prevent 
> inlining introduced by recent PR56727 patch.
> 
> I added Power-specific pattern in pr56727-2.c testcase and disabled 
> testing on Power in pr56727-1.c.
> 
> Tested on powerpc64-unknown-linux-gnu.  Ok for trunk?

Did you also test this with -m32?  And on powerpc64le-linux, the target
the bug is reported against?  The three ABIs are different.

> 	PR target/81535
> 	* gcc.dg/pr56727-1.c: Do not check output on Power.
> 	* gcc.dg/pr56727-2.c: Fix pattern for Power.

Please name the actual target triple here, i.e. powerpc*-*-* .

> 	* gcc.target/powerpc/pr79439.c: Prevent inlining.

> diff -rupN gcc/gcc/testsuite/gcc.dg/pr56727-1.c gcc-81535/gcc/testsuite/gcc.dg/pr56727-1.c
> --- gcc/gcc/testsuite/gcc.dg/pr56727-1.c	2017-07-28 02:39:54.770046466 +0000
> +++ gcc-81535/gcc/testsuite/gcc.dg/pr56727-1.c	2017-07-28 04:25:04.805648587 +0000
> @@ -1,6 +1,6 @@
>  /* { dg-do compile { target fpic } } */
>  /* { dg-options "-O2 -fPIC" } */
> -/* { dg-final { scan-assembler-not "@(PLT|plt)" { target i?86-*-* x86_64-*-* powerpc*-*-* } } } */
> +/* { dg-final { scan-assembler-not "@(PLT|plt)" { target i?86-*-* x86_64-*-* } } } */
>  
>  #define define_func(type) \
>    void f_ ## type (type b) { f_ ## type (0); } \

Is it correct that current GCC does not do the call via the PLT?

> diff -rupN gcc/gcc/testsuite/gcc.dg/pr56727-2.c gcc-81535/gcc/testsuite/gcc.dg/pr56727-2.c
> --- gcc/gcc/testsuite/gcc.dg/pr56727-2.c	2017-07-28 02:39:54.770046466 +0000
> +++ gcc-81535/gcc/testsuite/gcc.dg/pr56727-2.c	2017-07-28 04:21:19.195215187 +0000
> @@ -1,10 +1,10 @@
>  /* { dg-do compile { target fpic } } */
>  /* { dg-options "-O2 -fPIC" } */
> -/* { dg-final { scan-assembler "@(PLT|plt)" { target i?86-*-* x86_64-*-* powerpc*-*-linux* } } } */
>  
>  __attribute__((noinline, noclone))
>  void f (short b)
>  {
> +  __builtin_setjmp (0);  /* Prevent tailcall */
>    f (0);
>  }
>  

This change is not mentioned in the changelog.

> @@ -14,3 +14,5 @@ void h ()
>  {
>    g (0);
>  }
> +/* { dg-final { scan-assembler "@(PLT|plt)" { target i?86-*-* x86_64-*-* } } } */
> +/* { dg-final { scan-assembler "bl f\n\[ \t\]*nop" { target powerpc*-*-linux* } } } */

Is there a real reason there cannot be a tailcall here?  You can write
this as  { scan-assembler {\mbl f\s+nop\M} }  btw.

> diff -rupN gcc/gcc/testsuite/gcc.target/powerpc/pr79439.c gcc-81535/gcc/testsuite/gcc.target/powerpc/pr79439.c
> --- gcc/gcc/testsuite/gcc.target/powerpc/pr79439.c	2017-07-28 02:39:55.750048426 +0000
> +++ gcc-81535/gcc/testsuite/gcc.target/powerpc/pr79439.c	2017-07-28 04:13:47.834177237 +0000
> @@ -8,22 +8,17 @@
>  
>  int f (void);
>  
> -void
> -g (void)
> -{
> -}
> -
>  int
>  rec (int a)
>  {
>    int ret = 0;
>    if (a > 10 && f ())
>      ret += rec (a - 1);
> -  g ();
>    return a + ret;
>  }
>  
> +void rec_alias (short) __attribute__ ((alias ("rec")));
> +
>  /* { dg-final { scan-assembler-times {\mbl f\M}   1 } } */
> -/* { dg-final { scan-assembler-times {\mbl g\M}   1 } } */
>  /* { dg-final { scan-assembler-times {\mbl rec\M} 1 } } */
> -/* { dg-final { scan-assembler-times {\mnop\M}    3 } } */
> +/* { dg-final { scan-assembler-times {\mnop\M}    2 } } */

The changelog says "prevent inlining", but you actually delete the code.
Is that okay, wasn't the testcase actually testing for something here?


Segher

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH][PR target/81535] Fix tests on Power
  2017-08-07 23:33 ` [PATCH][PR " Segher Boessenkool
@ 2017-11-25 10:00   ` Yury Gribov
  0 siblings, 0 replies; 4+ messages in thread
From: Yury Gribov @ 2017-11-25 10:00 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: GCC Patches, seurer, wschmidt, meissner, marxin

On Tue, Aug 8, 2017 at 12:32 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> Hi Yuri,
>
> Sorry I missed this.  Please cc: me to prevent that from happening.

Segher,

Sorry, somehow I missed your reply!

> On Fri, Jul 28, 2017 at 05:42:00AM +0100, Yury Gribov wrote:
>> This patch fixes issues reported in
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81535
>>
>> I removed call to g in pr79439.c because gcc was duplicating the basic
>> block with call depending on compiler flags (so scan-assembler-times
>> pattern wasn't reliable anymore).  I also added alias to prevent
>> inlining introduced by recent PR56727 patch.
>>
>> I added Power-specific pattern in pr56727-2.c testcase and disabled
>> testing on Power in pr56727-1.c.
>>
>> Tested on powerpc64-unknown-linux-gnu.  Ok for trunk?
>
> Did you also test this with -m32?  And on powerpc64le-linux, the target
> the bug is reported against?  The three ABIs are different.

No, only tested standard powerpc64. I'll go check other targets then.

>>       PR target/81535
>>       * gcc.dg/pr56727-1.c: Do not check output on Power.
>>       * gcc.dg/pr56727-2.c: Fix pattern for Power.
>
> Please name the actual target triple here, i.e. powerpc*-*-* .

Makes sense.

>>       * gcc.target/powerpc/pr79439.c: Prevent inlining.
>
>> diff -rupN gcc/gcc/testsuite/gcc.dg/pr56727-1.c gcc-81535/gcc/testsuite/gcc.dg/pr56727-1.c
>> --- gcc/gcc/testsuite/gcc.dg/pr56727-1.c      2017-07-28 02:39:54.770046466 +0000
>> +++ gcc-81535/gcc/testsuite/gcc.dg/pr56727-1.c        2017-07-28 04:25:04.805648587 +0000
>> @@ -1,6 +1,6 @@
>>  /* { dg-do compile { target fpic } } */
>>  /* { dg-options "-O2 -fPIC" } */
>> -/* { dg-final { scan-assembler-not "@(PLT|plt)" { target i?86-*-* x86_64-*-* powerpc*-*-* } } } */
>> +/* { dg-final { scan-assembler-not "@(PLT|plt)" { target i?86-*-* x86_64-*-* } } } */
>>
>>  #define define_func(type) \
>>    void f_ ## type (type b) { f_ ## type (0); } \
>
> Is it correct that current GCC does not do the call via the PLT?

Well, it was decided in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56727 that it would be a
valid optimization because the only way to expose the difference would
be through dlsym hackery. Note that original PowerPC use-case
(reported in https://sourceware.org/bugzilla/show_bug.cgi?id=21116)
would benefit from this optimization as because PLT call + indirection
would be replaced by normal PC-relative call.

>> diff -rupN gcc/gcc/testsuite/gcc.dg/pr56727-2.c gcc-81535/gcc/testsuite/gcc.dg/pr56727-2.c
>> --- gcc/gcc/testsuite/gcc.dg/pr56727-2.c      2017-07-28 02:39:54.770046466 +0000
>> +++ gcc-81535/gcc/testsuite/gcc.dg/pr56727-2.c        2017-07-28 04:21:19.195215187 +0000
>> @@ -1,10 +1,10 @@
>>  /* { dg-do compile { target fpic } } */
>>  /* { dg-options "-O2 -fPIC" } */
>> -/* { dg-final { scan-assembler "@(PLT|plt)" { target i?86-*-* x86_64-*-* powerpc*-*-linux* } } } */
>>
>>  __attribute__((noinline, noclone))
>>  void f (short b)
>>  {
>> +  __builtin_setjmp (0);  /* Prevent tailcall */
>>    f (0);
>>  }
>>
>
> This change is not mentioned in the changelog.

Well, this was a minor hack to prevent tailcalling, I thought it does
not deserve a comment...

>> @@ -14,3 +14,5 @@ void h ()
>>  {
>>    g (0);
>>  }
>> +/* { dg-final { scan-assembler "@(PLT|plt)" { target i?86-*-* x86_64-*-* } } } */
>> +/* { dg-final { scan-assembler "bl f\n\[ \t\]*nop" { target powerpc*-*-linux* } } } */
>
> Is there a real reason there cannot be a tailcall here?  You can write
> this as  { scan-assembler {\mbl f\s+nop\M} }  btw.

Yes, setjmp will block tailcall optimization. Will fix the pattern, thanks!

>> diff -rupN gcc/gcc/testsuite/gcc.target/powerpc/pr79439.c gcc-81535/gcc/testsuite/gcc.target/powerpc/pr79439.c
>> --- gcc/gcc/testsuite/gcc.target/powerpc/pr79439.c    2017-07-28 02:39:55.750048426 +0000
>> +++ gcc-81535/gcc/testsuite/gcc.target/powerpc/pr79439.c      2017-07-28 04:13:47.834177237 +0000
>> @@ -8,22 +8,17 @@
>>
>>  int f (void);
>>
>> -void
>> -g (void)
>> -{
>> -}
>> -
>>  int
>>  rec (int a)
>>  {
>>    int ret = 0;
>>    if (a > 10 && f ())
>>      ret += rec (a - 1);
>> -  g ();
>>    return a + ret;
>>  }
>>
>> +void rec_alias (short) __attribute__ ((alias ("rec")));
>> +
>>  /* { dg-final { scan-assembler-times {\mbl f\M}   1 } } */
>> -/* { dg-final { scan-assembler-times {\mbl g\M}   1 } } */
>>  /* { dg-final { scan-assembler-times {\mbl rec\M} 1 } } */
>> -/* { dg-final { scan-assembler-times {\mnop\M}    3 } } */
>> +/* { dg-final { scan-assembler-times {\mnop\M}    2 } } */
>
> The changelog says "prevent inlining", but you actually delete the code.
> Is that okay, wasn't the testcase actually testing for something here?

TBH at this point I don't remember. I think call to g() didn't add
anything to the test so I removed it. Let me comment on this once I
retest the patch as discussed above.

-Y

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-11-25  8:51 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-28  4:42 [PATCH][PR target/81535] Fix tests on Power Yury Gribov
2017-08-04 20:35 ` [PATCH][PING][PR " Yury Gribov
2017-08-07 23:33 ` [PATCH][PR " Segher Boessenkool
2017-11-25 10:00   ` Yury Gribov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).