If a sincos instruction is defined, it is profitable to introduce these foldings: sincos (x, &s, &c); sincos (-x, &s, &c); to (*c = cos (x), *s = sin (x)) (*c = cos (x), *s = -sin (x)) respectively. This allows the sincos instruction to be used; the second folding is necessary to avoid that the cos (x) = cos (-x) optimization kicks in, leaving us with an unoptimizable (*c = cos (x), *s = sin (-x)) This patch is proposed for 4.1 through the 17652 metabug.
*** Bug 17686 has been marked as a duplicate of this bug. ***
Created attachment 7223 [details] patch to introduce fold_builtin_sincos bootstrapped/regtested i686-pc-linux-gnu, includes 2 testcases
Subject: Bug 17687 CVSROOT: /cvs/gcc Module name: gcc Changes by: mmitchel@gcc.gnu.org 2004-10-10 05:02:54 Modified files: gcc/testsuite : ChangeLog gcc/cp : ChangeLog error.c init.c parser.c tree.c Added files: gcc/testsuite/g++.dg/init: new11.C gcc/testsuite/g++.dg/parse: error19.C error20.C gcc/testsuite/g++.dg/template: crash24.C Log message: PR c++/17867 * error.c (dump_expr): Correct handling of AGGR_INIT_EXPRs using a constructor. PR c++/17670 * init.c (build_new): Correct comments. * parser.c (cp_parser_new_expression): Use NULL_TREE for nelts in the non-array case. PR c++/17821 * parser.c (cp_parser_postfix_dot_deref_expression): If the pseduo-destructor-name production does not work, fall back to the ordinary production. PR c++/17826 * tree.c (cp_tree_equal): Handle a BASELINK. PR c++/17687 * g++.dg/parse/error19.C: New test. PR c++/17670 * g++.dg/init/new11.C: New test. PR c++/17821 * g++.dg/parse/error20.C: New test. PR c++/17826 * g++.dg/template/crash24.C: New test. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.4426&r2=1.4427 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/ChangeLog.diff?cvsroot=gcc&r1=1.4416&r2=1.4417 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/error.c.diff?cvsroot=gcc&r1=1.266&r2=1.267 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/init.c.diff?cvsroot=gcc&r1=1.398&r2=1.399 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/parser.c.diff?cvsroot=gcc&r1=1.261&r2=1.262 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/tree.c.diff?cvsroot=gcc&r1=1.413&r2=1.414 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/init/new11.C.diff?cvsroot=gcc&r1=NONE&r2=1.1 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/parse/error19.C.diff?cvsroot=gcc&r1=NONE&r2=1.1 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/parse/error20.C.diff?cvsroot=gcc&r1=NONE&r2=1.1 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/template/crash24.C.diff?cvsroot=gcc&r1=NONE&r2=1.1
Subject: Bug 17687 CVSROOT: /cvs/gcc Module name: gcc Branch: gcc-3_4-branch Changes by: mmitchel@gcc.gnu.org 2004-10-10 05:28:32 Modified files: gcc/testsuite : ChangeLog gcc/cp : ChangeLog parser.c tree.c Added files: gcc/testsuite/g++.dg/parse: error20.C gcc/testsuite/g++.dg/template: crash24.C Log message: PR c++/17867 * error.c (dump_expr): Correct handling of AGGR_INIT_EXPRs using a constructor. PR c++/17670 * init.c (build_new): Correct comments. * parser.c (cp_parser_new_expression): Use NULL_TREE for nelts in the non-array case. PR c++/17821 * parser.c (cp_parser_postfix_dot_deref_expression): If the pseduo-destructor-name production does not work, fall back to the ordinary production. PR c++/17826 * tree.c (cp_tree_equal): Handle a BASELINK. PR c++/17687 * g++.dg/parse/error19.C: New test. PR c++/17670 * g++.dg/init/new11.C: New test. PR c++/17821 * g++.dg/parse/error20.C: New test. PR c++/17826 * g++.dg/template/crash24.C: New test. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.3389.2.281&r2=1.3389.2.282 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.3892.2.165&r2=1.3892.2.166 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/parser.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.157.2.42&r2=1.157.2.43 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/tree.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.360.4.9&r2=1.360.4.10 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/parse/error20.C.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=NONE&r2=1.1.2.1 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/template/crash24.C.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=NONE&r2=1.1.2.1
Paolo, are you going to submit this one?
Subject: Re: [4.1] sincos can be folded at the tree level > Paolo, are you going to submit this one? Yes, but I am waaaay too busy at work now. Maybe as soon as Thursday.
Was this implemented? There is an expand_builtin_sincos, but I think it cannot remove TREE_ADDRESSABLE on the arguments, making most of the performance advantages go away...
I implemented the expander to allow the x87 backend using fsincos for it. The problems (TREE_ADDRESSABLE, no real folding, no CSE with previous sin/cos) still exist. I had/have (I don't remember if I submitted them) patches to canonicalize sin/cos/sincos to cexp and expand cexp to sin/cos/sincos dependent on arguments. If this PR was only about x87 using fsincos for sincos this is fixed now.
Subject: Re: sincos can be folded at the tree level > If this PR was only about x87 using fsincos for sincos this is fixed now. Well, it was mostly about getting rid of TREE_ADDRESSABLE, which you can really do efficiently only on x87, using fsincos. But maybe it's time to change the subject.
see patch at http://gcc.gnu.org/ml/gcc-patches/2005-12/msg01151.html
If all that we care about is TREE_ADDRESSABLE, and not folding together with a previous sin/cos call, we may just change sincos (x, &s, &c); to sincos (x, &t1, &t2); s = t1; c = t2; maybe?
Created attachment 12055 [details] patch using cexp Well, then you make t1 and t2 addressable. We could introduce a __builtin_sane_sincos which returns in a vector, but then at expand time we'd have the same problems if we ever want to go back to a sincos libcall. With libgcc-math we could provide one with a sane interface though.
sure, but t1 and t2 die the moment they are assigned back. it would be just a trick to return in memory, but not make s and c addressable all the way down to RA. though i don't remember how big a penalty is if your variable is addressable just for the sake of one or two uses.
This PR is only about a non-optimal tree-representation for __builtin_sincos. See also http://gcc.gnu.org/ml/gcc-patches/2005-12/msg01151.html for an alternative and some discussion. (The other patch was rejected, I believe further discussion is necessary here. For reference: http://gcc.gnu.org/ml/gcc-patches/2005-03/msg01571.html )
Paolo, are you working on this?
Subject: Re: sincos tree representation causes extra addressable vars > Paolo, are you working on this? No. :-(
So I hope you don't mind...
Subject: Bug number PR17687 A patch for this bug has been added to the patch tracker. The mailing list url for the patch is http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00476.html
Subject: Bug number PR17687 A patch for this bug has been added to the patch tracker. The mailing list url for the patch is http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00536.html
Fixed.