[PATCH][testsuite] Abort on failure in gcc.dg/pr30957-1.c

[PATCH][testsuite] Abort on failure in gcc.dg/pr30957-1.c

[Tom de Vries]

Hi,

this patch reverses the abort logic in pr30957-1.c, such that it aborts on 
failure rather than on success.

OK for stage4?

Thanks,
- Tom

2015-02-20  Tom de Vries  <tom@codesourcery.com>

	* gcc.dg/pr30957-1.c (main): Abort on failure.
---
  gcc/testsuite/gcc.dg/pr30957-1.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/pr30957-1.c b/gcc/testsuite/gcc.dg/pr30957-1.c
index 65b98fa..45fb6d6 100644
--- a/gcc/testsuite/gcc.dg/pr30957-1.c
+++ b/gcc/testsuite/gcc.dg/pr30957-1.c
@@ -1,4 +1,4 @@
-/* { dg-do run { xfail *-*-* } } */
+/* { dg-do run } */
  /* We don't (and don't want to) perform this optimisation on soft-float
     targets, where each addition is a library call.  This test requires
     -fassociative-math for enabling the variable-expansion as well as
@@ -27,8 +27,8 @@ int
  main ()
  {
    if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != -1.0)
-    abort ();
-  exit (0);
+    exit (0);
+  abort ();
  }

  /* { dg-final { scan-rtl-dump "Expanding Accumulator" "loop2_unroll" } } */
-- 
1.9.1

Re: [PATCH][testsuite] Abort on failure in gcc.dg/pr30957-1.c

[Jakub Jelinek]

On Fri, Feb 20, 2015 at 10:25:54AM +0100, Tom de Vries wrote:
> this patch reverses the abort logic in pr30957-1.c, such that it aborts on
> failure rather than on success.

That sounds really weird.  From the description it looks like it is a known bug
that we don't return -0.0.
If 0.0 is the right return value instead, I'd the test should be written as
if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
  abort ();
to make it clear you are expecting positive 0.

	Jakub

Re: [PATCH][testsuite] Abort on failure in gcc.dg/pr30957-1.c

[Tom de Vries]

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

On 20-02-15 10:42, Jakub Jelinek wrote:
> On Fri, Feb 20, 2015 at 10:25:54AM +0100, Tom de Vries wrote:
>> this patch reverses the abort logic in pr30957-1.c, such that it aborts on
>> failure rather than on success.
>
> That sounds really weird.  From the description it looks like it is a known bug
> that we don't return -0.0.
> If 0.0 is the right return value instead, I'd the test should be written as
> if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
>    abort ();
> to make it clear you are expecting positive 0.
>

Updated patch accordingly. OK for stage1?

Thanks,
- Tom

[-- Attachment #2: 0001-Make-pr30957-1.c-pass-rather-xfail.patch --]
[-- Type: text/x-patch, Size: 1842 bytes --]

2015-02-20  Tom de Vries  <tom@codesourcery.com>

	* gcc.dg/pr30957-1.c: Make pr30957-1.c pass rather xfail.
---
 gcc/testsuite/gcc.dg/pr30957-1.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/pr30957-1.c b/gcc/testsuite/gcc.dg/pr30957-1.c
index 65b98fa..0ed150d 100644
--- a/gcc/testsuite/gcc.dg/pr30957-1.c
+++ b/gcc/testsuite/gcc.dg/pr30957-1.c
@@ -1,10 +1,6 @@
-/* { dg-do run { xfail *-*-* } } */
-/* We don't (and don't want to) perform this optimisation on soft-float
-   targets, where each addition is a library call.  This test requires
-   -fassociative-math for enabling the variable-expansion as well as
-   -fsigned-zeros for honoring the sign of zero; but
-   they can not co-exist; also under -funsafe-math-optimizations, so we
-   expect it to fail.  */
+/* { dg-do run } */
+/* We don't (and don't want to) perform this optimisation on soft-float targets,
+   where each addition is a library call.  /
 /* { dg-require-effective-target hard_float } */
 /* { dg-options "-O2 -funroll-loops -funsafe-math-optimizations -fvariable-expansion-in-unroller -fdump-rtl-loop2_unroll" } */
 
@@ -26,12 +22,14 @@ foo (float d, int n)
 int
 main ()
 {
-  if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != -1.0)
+  /* When compiling standard compliant we expect foo to return -0.0.  But the
+     variable expansion during unrolling optimization (for this testcase enabled
+     by -funsafe-math-optimization) instantiates copy(s) of the accumulator
+     which it initializes with +0.0.  Hence we expect that foo returns +0.0.  */
+  if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
     abort ();
   exit (0);
 }
 
 /* { dg-final { scan-rtl-dump "Expanding Accumulator" "loop2_unroll" } } */
 /* { dg-final { cleanup-rtl-dump "loop*" } } */
-
-
-- 
1.9.1

Re: [PATCH][testsuite] Abort on failure in gcc.dg/pr30957-1.c

[Jakub Jelinek]

On Fri, Feb 20, 2015 at 01:36:15PM +0100, Tom de Vries wrote:
> On 20-02-15 10:42, Jakub Jelinek wrote:
> >On Fri, Feb 20, 2015 at 10:25:54AM +0100, Tom de Vries wrote:
> >>this patch reverses the abort logic in pr30957-1.c, such that it aborts on
> >>failure rather than on success.
> >
> >That sounds really weird.  From the description it looks like it is a known bug
> >that we don't return -0.0.
> >If 0.0 is the right return value instead, I'd the test should be written as
> >if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
> >   abort ();
> >to make it clear you are expecting positive 0.
> >
> 
> Updated patch accordingly. OK for stage1?

If it is a known bug, it should better stay as xfail, rather than pretending
the wrong behavior is correct.

	Jakub

Re: [PATCH][testsuite] Abort on failure in gcc.dg/pr30957-1.c

[Mike Stump]

On Feb 20, 2015, at 4:36 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 20-02-15 10:42, Jakub Jelinek wrote:
>> On Fri, Feb 20, 2015 at 10:25:54AM +0100, Tom de Vries wrote:
>>> this patch reverses the abort logic in pr30957-1.c, such that it aborts on
>>> failure rather than on success.
>> 
>> That sounds really weird.  From the description it looks like it is a known bug
>> that we don't return -0.0.
>> If 0.0 is the right return value instead, I'd the test should be written as
>> if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
>>   abort ();
>> to make it clear you are expecting positive 0.
>> 
> 
> Updated patch accordingly. OK for stage1?

I’ve tried to read through the bug report and all the patches, who did them and why…  The entire thread is messier than I’d like, which makes dealing with this whole thing messy.  The bug report I marked as fixed, as I think it now works as the bug reporter expects.  Seems like a mistake it wasn’t closed a while ago.

I now see why you went with this patch.  Why wait for stage 1…  Lets just put it in now and put an end to the misery.

Ok for trunk now.

Re: [PATCH][testsuite] Abort on failure in gcc.dg/pr30957-1.c

[Tom de Vries]

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

On 20-02-15 20:18, Mike Stump wrote:
> On Feb 20, 2015, at 4:36 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> On 20-02-15 10:42, Jakub Jelinek wrote:
>>> On Fri, Feb 20, 2015 at 10:25:54AM +0100, Tom de Vries wrote:
>>>> this patch reverses the abort logic in pr30957-1.c, such that it aborts on
>>>> failure rather than on success.
>>>
>>> That sounds really weird.  From the description it looks like it is a known bug
>>> that we don't return -0.0.
>>> If 0.0 is the right return value instead,

The behaviour of the compiler matches the semantics of -fassociative-math, so in 
that sense it's not a bug. I'm not how to interpret the 'right' value in case of 
a non-compliant optimization. I guess it's more a case of 'not wrong'.

>>> I'd the test should be written as
>>> if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
>>>    abort ();
>>> to make it clear you are expecting positive 0.
>>>
>>
>> Updated patch accordingly. OK for stage1?
>
> IÂ’ve tried to read through the bug report and all the patches, who did them and whyÂ…  The entire thread is messier than IÂ’d like, which makes dealing with this whole thing messy.  The bug report I marked as fixed, as I think it now works as the bug reporter expects.  Seems like a mistake it wasnÂ’t closed a while ago.

I've updated the PR with the status as I understand it, and marked it waiting 
(for review).

> I now see why you went with this patch.  Why wait for stage 1Â…  Lets just put it  in now and put an end to the misery.
>
> Ok for trunk now.
>

Committed as attached (using the more minimal -fassociative-math instead of 
-funsafe-math-optimizations).

Thanks,
- Tom

[-- Attachment #2: 0001-Make-pr30957-1.c-pass-rather-xfail.patch --]
[-- Type: text/x-patch, Size: 2144 bytes --]

2015-02-22  Tom de Vries  <tom@codesourcery.com>

	* gcc.dg/pr30957-1.c: Make pr30957-1.c pass rather xfail.
---
 gcc/testsuite/gcc.dg/pr30957-1.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/pr30957-1.c b/gcc/testsuite/gcc.dg/pr30957-1.c
index 65b98fa..f34c6e5 100644
--- a/gcc/testsuite/gcc.dg/pr30957-1.c
+++ b/gcc/testsuite/gcc.dg/pr30957-1.c
@@ -1,12 +1,9 @@
-/* { dg-do run { xfail *-*-* } } */
-/* We don't (and don't want to) perform this optimisation on soft-float
-   targets, where each addition is a library call.  This test requires
-   -fassociative-math for enabling the variable-expansion as well as
-   -fsigned-zeros for honoring the sign of zero; but
-   they can not co-exist; also under -funsafe-math-optimizations, so we
-   expect it to fail.  */
+/* { dg-do run } */
+/* We don't (and don't want to) perform this optimisation on soft-float targets,
+   where each addition is a library call.  /
 /* { dg-require-effective-target hard_float } */
-/* { dg-options "-O2 -funroll-loops -funsafe-math-optimizations -fvariable-expansion-in-unroller -fdump-rtl-loop2_unroll" } */
+/* -fassociative-math requires -fno-trapping-math and -fno-signed-zeros. */
+/* { dg-options "-O2 -funroll-loops -fassociative-math -fno-trapping-math -fno-signed-zeros -fvariable-expansion-in-unroller -fdump-rtl-loop2_unroll" } */
 
 extern void abort (void);
 extern void exit (int);
@@ -26,12 +23,15 @@ foo (float d, int n)
 int
 main ()
 {
-  if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != -1.0)
+  /* When compiling standard compliant we expect foo to return -0.0.  But the
+     variable expansion during unrolling optimization (for this testcase enabled
+     by non-compliant -fassociative-math) instantiates copy(s) of the
+     accumulator which it initializes with +0.0.  Hence we expect that foo
+     returns +0.0.  */
+  if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
     abort ();
   exit (0);
 }
 
 /* { dg-final { scan-rtl-dump "Expanding Accumulator" "loop2_unroll" } } */
 /* { dg-final { cleanup-rtl-dump "loop*" } } */
-
-
-- 
1.9.1

Re: [PATCH][testsuite] Abort on failure in gcc.dg/pr30957-1.c

[Mike Stump]

On Feb 20, 2015, at 1:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Feb 20, 2015 at 10:25:54AM +0100, Tom de Vries wrote:
>> this patch reverses the abort logic in pr30957-1.c, such that it aborts on
>> failure rather than on success.
> 
> That sounds really weird.  From the description it looks like it is a known bug
> that we don't return -0.0.
> If 0.0 is the right return value instead, I'd the test should be written as
> if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
>  abort ();
> to make it clear you are expecting positive 0.

So, did you read the bug report?  They expect the value -1.0, so, I think the above is wrong?

Re: [PATCH][testsuite] Abort on failure in gcc.dg/pr30957-1.c

[Mike Stump]

On Feb 20, 2015, at 10:12 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Feb 20, 2015, at 1:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Fri, Feb 20, 2015 at 10:25:54AM +0100, Tom de Vries wrote:
>>> this patch reverses the abort logic in pr30957-1.c, such that it aborts on
>>> failure rather than on success.
>> 
>> That sounds really weird.  From the description it looks like it is a known bug
>> that we don't return -0.0.
>> If 0.0 is the right return value instead, I'd the test should be written as
>> if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
>> abort ();
>> to make it clear you are expecting positive 0.
> 
> So, did you read the bug report?  They expect the value -1.0, so, I think the above is wrong?

Ignore that…  I’ve read though more of the history and the whole think is just messier than I’d like.  I now see why Tom proposed the patch he did.