| Message ID | 87h9lsi81p.fsf@e105548-lin.cambridge.arm.com |
|---|---|
| State | New |
| Headers | show |
On Oct 15, 2015, at 6:18 AM, Richard Sandiford <richard.sandiford@arm.com> wrote: > Stripping unnecessary sign ops at the gimple level means that we're > no longer able to optimise: > > if (cos(y<10 ? -fabs(x) : tan(x<20 ? -x : -fabs(y))) > != cos(y<10 ? x : tan(x<20 ? x : y))) > link_error (); > > because we're currently not able to fold away the equality in: > > int > f1 (double x, double y) > { > double z1 = __builtin_cos(y<10 ? x : __builtin_tan(x<20 ? x : y)); > double z2 = __builtin_cos(y<10 ? x : __builtin_tan(x<20 ? x : y)); > return z1 == z2; > } > > The missed fold is being tracked as PR 67975. This patch splits the > test out into a separate file so that we can XFAIL it until the PR > is fixed. So, with my peanut gallery hat on, I’m not a fan of this sort of thing. If others want to do this though, I’d let that be their call. As a counter example, I’d be fine with #if 0ing the tests that don’t work, and putting in the PR the reserve of that patch to remove the added #if 0 when that PR is fixed. The test case remains complete, and once the PR is fixed, the test suite will be as it was. Having tests that we know fail doesn’t enhance the quality of the compiler any. It is a nice tracking mechanism, if no PR exists, but, when a PR exists, we don’t need the test case to track the issue, as the PR will. In general, adding #if 0 to tests is undesirable, but, when one cannot toggle subtests easily or mark subtest failures easily, I prefer it over splitting.
Mike Stump <mikestump@comcast.net> writes: > On Oct 15, 2015, at 6:18 AM, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> Stripping unnecessary sign ops at the gimple level means that we're >> no longer able to optimise: >> >> if (cos(y<10 ? -fabs(x) : tan(x<20 ? -x : -fabs(y))) >> != cos(y<10 ? x : tan(x<20 ? x : y))) >> link_error (); >> >> because we're currently not able to fold away the equality in: >> >> int >> f1 (double x, double y) >> { >> double z1 = __builtin_cos(y<10 ? x : __builtin_tan(x<20 ? x : y)); >> double z2 = __builtin_cos(y<10 ? x : __builtin_tan(x<20 ? x : y)); >> return z1 == z2; >> } >> >> The missed fold is being tracked as PR 67975. This patch splits the >> test out into a separate file so that we can XFAIL it until the PR >> is fixed. > > So, with my peanut gallery hat on, I’m not a fan of this sort of thing. > If others want to do this though, I’d let that be their call. > > As a counter example, I’d be fine with #if 0ing the tests that don’t > work, and putting in the PR the reserve of that patch to remove the > added #if 0 when that PR is fixed. The test case remains complete, and > once the PR is fixed, the test suite will be as it was. Having tests > that we know fail doesn’t enhance the quality of the compiler any. It > is a nice tracking mechanism, if no PR exists, but, when a PR exists, we > don’t need the test case to track the issue, as the PR will. In > general, adding #if 0 to tests is undesirable, but, when one cannot > toggle subtests easily or mark subtest failures easily, I prefer it over > splitting. I can see that argument if people are only taking work items from the PR database. But it's possible (likely even) that people will independently find a problem like this and just fix it, if the missed optimisation happens to be important to them. I don't think they should then have to trawl the PR database to see which PRs their patch fixes. XFAILs are a nice automated way of making it obvious. (The patch that adds the XFAIL also adds a comment pointing to the associated PR.) Thanks, Richard
On Oct 15, 2015, at 12:47 PM, Richard Sandiford <richard.sandiford@arm.com> wrote: > I can see that argument if people are only taking work items from > the PR database. But it's possible (likely even) that people will > independently find a problem like this and just fix it, if the missed > optimisation happens to be important to them. I don't think they > should then have to trawl the PR database to see which PRs their patch > fixes. There is no requirement that they do.
Mike Stump <mikestump@comcast.net> writes: > On Oct 15, 2015, at 12:47 PM, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> I can see that argument if people are only taking work items from >> the PR database. But it's possible (likely even) that people will >> independently find a problem like this and just fix it, if the missed >> optimisation happens to be important to them. I don't think they >> should then have to trawl the PR database to see which PRs their patch >> fixes. > > There is no requirement that they do. But if they don't the original test stays #if 0d out. I don't see why that's better than having an XFAIL become an XPASS, so that it's obvious that the XFAIL can be removed and we get the test back quicker.
On Oct 15, 2015, at 1:38 PM, Richard Sandiford <richard.sandiford@arm.com> wrote: > Mike Stump <mikestump@comcast.net> writes: >> On Oct 15, 2015, at 12:47 PM, Richard Sandiford >> <richard.sandiford@arm.com> wrote: >>> I can see that argument if people are only taking work items from >>> the PR database. But it's possible (likely even) that people will >>> independently find a problem like this and just fix it, if the missed >>> optimisation happens to be important to them. I don't think they >>> should then have to trawl the PR database to see which PRs their patch >>> fixes. >> >> There is no requirement that they do. > > But if they don't the original test stays #if 0d out. Only until someone retests the PR. > I don't see why that's better than having an XFAIL become an XPASS, so that it's > obvious that the XFAIL can be removed and we get the test back quicker. I have a slight preference to not split the test cases in this case. As I said, it isn’t a big deal, and if you feel it is better to split the test case, then your patch is ok. I think it is trivial as I don’t know of a reason why someone should reject it.
On 10/15/2015 07:18 AM, Richard Sandiford wrote: > Stripping unnecessary sign ops at the gimple level means that we're > no longer able to optimise: > > if (cos(y<10 ? -fabs(x) : tan(x<20 ? -x : -fabs(y))) > != cos(y<10 ? x : tan(x<20 ? x : y))) > link_error (); > > because we're currently not able to fold away the equality in: > > int > f1 (double x, double y) > { > double z1 = __builtin_cos(y<10 ? x : __builtin_tan(x<20 ? x : y)); > double z2 = __builtin_cos(y<10 ? x : __builtin_tan(x<20 ? x : y)); > return z1 == z2; > } > > The missed fold is being tracked as PR 67975. This patch splits the > test out into a separate file so that we can XFAIL it until the PR > is fixed. > > Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi. > OK to install? > > Thanks, > Richard > > > gcc/testsuite/ > * gcc.dg/builtins-20.c: Move some tests to... > * gcc.dg/builtins-86.c: ...this new file. Yes. I just went through this in a totally unrelated area. I'd much rather have the test split out and xfailed. jeff
diff --git a/gcc/testsuite/gcc.dg/builtins-20.c b/gcc/testsuite/gcc.dg/builtins-20.c index 2b63428..8288367 100644 --- a/gcc/testsuite/gcc.dg/builtins-20.c +++ b/gcc/testsuite/gcc.dg/builtins-20.c @@ -115,10 +115,6 @@ void test2(double x, double y) if (cos(y<10 ? x : -y) != cos(y<10 ? x : y)) link_error (); - if (cos(y<10 ? -fabs(x) : tan(x<20 ? -x : -fabs(y))) - != cos(y<10 ? x : tan(x<20 ? x : y))) - link_error (); - if (cos((y*=3, -x)) != cos((y*=3,x))) link_error (); @@ -343,10 +339,6 @@ void test2f(float x, float y) if (cosf(y<10 ? x : -y) != cosf(y<10 ? x : y)) link_error (); - if (cosf(y<10 ? -fabsf(x) : tanf(x<20 ? -x : -fabsf(y))) - != cosf(y<10 ? x : tanf(x<20 ? x : y))) - link_error (); - if (cosf((y*=3, -x)) != cosf((y*=3,x))) link_error (); @@ -570,10 +562,6 @@ void test2l(long double x, long double y) if (cosl(y<10 ? x : -y) != cosl(y<10 ? x : y)) link_error (); - if (cosl(y<10 ? -fabsl(x) : tanl(x<20 ? -x : -fabsl(y))) - != cosl(y<10 ? x : tanl(x<20 ? x : y))) - link_error (); - if (cosl((y*=3, -x)) != cosl((y*=3,x))) link_error (); diff --git a/gcc/testsuite/gcc.dg/builtins-86.c b/gcc/testsuite/gcc.dg/builtins-86.c new file mode 100644 index 0000000..f405124 --- /dev/null +++ b/gcc/testsuite/gcc.dg/builtins-86.c @@ -0,0 +1,56 @@ +/* Copyright (C) 2003-2015 Free Software Foundation. + + Split out from builtins-20.c, which has the comment: + + Verify that built-in math function constant folding doesn't break + anything and produces the expected results. + + Written by Roger Sayle, 8th June 2003. */ + +/* { dg-options "-O2 -ffast-math -fdump-tree-optimized" } */ + +#include "builtins-config.h" + +extern double cos (double); +extern double tan (double); +extern double fabs (double); +extern float cosf (float); +extern float tanf (float); +extern float fabsf (float); +extern long double cosl (long double); +extern long double tanl (long double); +extern long double fabsl (long double); + +extern void link_error(void); + +void test2(double x, double y) +{ + if (cos(y<10 ? -fabs(x) : tan(x<20 ? -x : -fabs(y))) + != cos(y<10 ? x : tan(x<20 ? x : y))) + link_error (); +} + +void test2f(float x, float y) +{ + if (cosf(y<10 ? -fabsf(x) : tanf(x<20 ? -x : -fabsf(y))) + != cosf(y<10 ? x : tanf(x<20 ? x : y))) + link_error (); +} + +void test2l(long double x, long double y) +{ + if (cosl(y<10 ? -fabsl(x) : tanl(x<20 ? -x : -fabsl(y))) + != cosl(y<10 ? x : tanl(x<20 ? x : y))) + link_error (); +} + +int main() +{ + test2 (1.0, 2.0); + test2f (1.0f, 2.0f); + test2l (1.0l, 2.0l); + + return 0; +} + +/* { dg-final { scan-tree-dump-times "link_error" 0 "optimized" } } */
Stripping unnecessary sign ops at the gimple level means that we're no longer able to optimise: if (cos(y<10 ? -fabs(x) : tan(x<20 ? -x : -fabs(y))) != cos(y<10 ? x : tan(x<20 ? x : y))) link_error (); because we're currently not able to fold away the equality in: int f1 (double x, double y) { double z1 = __builtin_cos(y<10 ? x : __builtin_tan(x<20 ? x : y)); double z2 = __builtin_cos(y<10 ? x : __builtin_tan(x<20 ? x : y)); return z1 == z2; } The missed fold is being tracked as PR 67975. This patch splits the test out into a separate file so that we can XFAIL it until the PR is fixed. Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi. OK to install? Thanks, Richard gcc/testsuite/ * gcc.dg/builtins-20.c: Move some tests to... * gcc.dg/builtins-86.c: ...this new file.