[Bug tree-optimization/30038] New: Call to sin(x), cos(x) should be transformed to sincos(x)

[Bug tree-optimization/30038] New: Call to sin(x), cos(x) should be transformed to sincos(x)

[burnus at gcc dot gnu dot org]

See also: http://gcc.gnu.org/ml/fortran/2006-12/msg00000.html

Tim Prince's analysis why ifort needs 8.77s and gfortran 12.16s for one
program:

"My copy of gfortran makes separate scalar calls to sin and cos, where ifort
makes a vector sincos call. 2 seconds to be gained there"

In the Fortran program below, a there is a call to the same argument with sin
and with cos. The resulting
  gfortran -msse3 -march=opteron -ffast-math -O3 -c -fdump-tree-optimized
shows
  pretmp.56 = __builtin_sinf (pretmp.80);
  pretmp.86 = __builtin_cosf (pretmp.80);
Using sincos should be faster.


Fortran program:

subroutine
test(number_of_sample_points,radius,coefficient,radius_of_curvature,spin_frequency,time,tmp)
  implicit none
  integer :: number_of_sample_points
  real, parameter   :: twopi = 6.28319
  integer :: n
  real :: radius(number_of_sample_points), coefficient, radius_of_curvature, &
          spin_frequency,time,tmp(2,number_of_sample_points)
  do n = 1, number_of_sample_points
    coefficient = (radius(n) / radius_of_curvature) * sin(twopi *            &
                                        spin_frequency * time)
    tmp(1,n) = coefficient
    coefficient = twopi * spin_frequency * (radius(n) /       &
                                     radius_of_curvature) * cos(twopi *      &
                                         spin_frequency * time)
    tmp(2,n) = coefficient
  end do
end subroutine


-- 
           Summary: Call to sin(x), cos(x) should be transformed to
                    sincos(x)
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Keywords: missed-optimization
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: burnus at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[ubizjak at gmail dot com]

------- Comment #1 from ubizjak at gmail dot com  2006-12-01 10:14 -------
Depends on PR tree_optimization/17687. I guess it is a time to finally resolve
that one...


-- 

ubizjak at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ubizjak at gmail dot com
  BugsThisDependsOn|                            |17687
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2006-12-01 10:14:36
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[bonzini at gnu dot org]

------- Comment #2 from bonzini at gnu dot org  2006-12-01 10:57 -------
Not only that; the glibc sincos, for example, would not gain anything.  ifort
gains because it basically computes the sine and cosine using two halves of an
XMM register.

Richard had a really gross ;-) patch to do this.


-- 

bonzini at gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[bonzini at gnu dot org]

------- Comment #3 from bonzini at gnu dot org  2006-12-01 11:04 -------
We need multiple steps here to solve this bug and 17687:

1) change sincos (x, &s, &c) to

     sincos (x, &t1, &t2);
     s = t1;
     c = t2;

Don't know the best place to do this.

2) alternatively, change sincos (x, &s, &c) to

     v2df = vec_sincos (x)
     *s = v2df[0]
     *c = v2df[1]

This needs to be done at tree->rtl expansion time, because we don't have access
to v2df[N] at tree level.

3) perform strength reduction of sin+cos to sincos.  I believe it has to be
done in a special pass (like CSE reciprocals), and that's actually why the file
was named tree-ssa-_math-opts_.c rather than tree-ssa-recip.c.  This could be
the place to do (1) too.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[rguenth at gcc dot gnu dot org]

------- Comment #4 from rguenth at gcc dot gnu dot org  2006-12-01 16:00 -------
My gross patch will still work ;)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[rguenth at gcc dot gnu dot org]

------- Comment #5 from rguenth at gcc dot gnu dot org  2006-12-01 16:03 -------
Btw, the "gross" patch is attached to PR17687.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[burnus at gcc dot gnu dot org]

------- Comment #6 from burnus at gcc dot gnu dot org  2006-12-01 18:05 -------
(In reply to comment #5)
> Btw, the "gross" patch is attached to PR17687.

If you mean attachment (id=12055; patch using cexp) then note that it no longer
applies cleanly:
2 out of 5 hunks FAILED -- saving rejects to file builtins.c.rej
1 out of 2 hunks FAILED -- saving rejects to file expr.c.rej


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[rguenth at gcc dot gnu dot org]

------- Comment #7 from rguenth at gcc dot gnu dot org  2006-12-06 09:52 -------
Ok, so my plan is to go in small steps:

 - at gimplification time change calls to sincos (x, &s, &c) to calls to cexp
like

    _Complex __typeof__(s) tmp = cexp ( { 0., x } );
    s = __imag tmp;
    c = __real tmp;

 - at expansion time, transform the above back to a call to sincos.  This
relies
   on TER to make the argument of form { 0., x } visible to the expander.
   Alternatively do this back-transformation before going out-of-ssa.
   2nd alternatively create a new (internal) builtin cexpi that takes the
   imaginary part of the argument only, i.e. cexpi (x) would be sincos.  This
   at least would simplify the code.

The above two should fix PR17687 on the tree level and on the rtl level where
the target provides either a sane ABI to the sincos function (or an
alternative)
or a machine instruction (like fsincos).

 - inside PRE recognize sin and cos and do insertion of cexp if necessary.
   If we do it in the "hackish" way, simply transform sin and cos to
   cexp before PRE and transform back cexp to sin or cos if only the real
   or imaginary parts of the result is used after PRE.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[rguenth at gcc dot gnu dot org]

------- Comment #8 from rguenth at gcc dot gnu dot org  2006-12-07 16:06 -------
I have a patch.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |rguenth at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2006-12-01 10:14:36         |2006-12-07 16:06:51
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[rguenth at gcc dot gnu dot org]

------- Comment #9 from rguenth at gcc dot gnu dot org  2006-12-07 16:07 -------
Created an attachment (id=12767)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=12767&action=view)
patch prototype


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[burnus at gcc dot gnu dot org]

------- Comment #10 from burnus at gcc dot gnu dot org  2006-12-07 20:00 -------
Using the three patches:
http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00500.html
http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00499.html
http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00476.html

gfortran is able to use sincos - and does so for my example (comment #0; the
example, however, cannot be vectorized).


For fatigue.f90 I have the following timings:
 gfortran: 21s
 ifort:    11s
 sunf95:    8.3s

The timings for gfortran don't change with the patch as sincos is NOT used.

If I make it obvious for the compiler that I have the same argument by using:
  mySin = sin( ... )
  myCos = cos( ... )
  expr = ... * mySin
then sincos is used and the execution time drops to 8.7s. (And remains
unchanged for ifort and sunf95.)

The unmodified source looks like:
  coefficient = (radius(n) / radius_of_curvature) &
                * sin(twopi * real(spin_frequency,LONGreal) * time)
  strain_tensor(1,1,n) = - coefficient * Poissons_ratio
  strain_tensor(2,2,n) = - coefficient * Poissons_ratio
  strain_tensor(3,3,n) = coefficient
!
  coefficient = twopi * real(spin_frequency,LONGreal) &
               * (radius(n) / radius_of_curvature) &
               * cos(twopi * real(spin_frequency,LONGreal) * time)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[dorit at il dot ibm dot com]

------- Comment #11 from dorit at il dot ibm dot com  2006-12-07 20:19 -------
(In reply to comment #10)
> Using the three patches:
...
> gfortran is able to use sincos - and does so for my example (comment #0; the
> example, however, cannot be vectorized).

 why? (what does -fdump-tree-vect-details say?)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[burnus at gcc dot gnu dot org]

------- Comment #12 from burnus at gcc dot gnu dot org  2006-12-07 20:32 -------
(In reply to comment #11)
> > gfortran is able to use sincos - and does so for my example (comment #0; the
> > example, however, cannot be vectorized).
>  why? (what does -fdump-tree-vect-details say?)
sinc.f90:8: note: not vectorized: relevant stmt not supported: D.1408_32 =
(*radius_31)[D.1407_30]


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[burnus at gcc dot gnu dot org]

------- Comment #13 from burnus at gcc dot gnu dot org  2006-12-07 20:36 -------
Created an attachment (id=12769)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=12769&action=view)
-fdump-tree-vect-details output of the example


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[rguenth at gcc dot gnu dot org]

------- Comment #14 from rguenth at gcc dot gnu dot org  2006-12-07 20:48 -------
sincos is not being used because we don't see that time is not aliased by the
writes to strain_tensor:

  #   VUSE <SMT.774_6041>;
  time.463_952 = time;
  D.3397_953 = time.463_952 *
6.283185307000000108246240415610373020172119140625e+0;
  D.3398_954 = D.3397_953 * D.3396_951;
  D.3399_955 = __builtin_sin (D.3398_954);
  coefficient_956 = D.3394_948 * D.3399_955;
  D.3279_957 = pretmp.823_106643;
  D.3400_958 = pretmp.824_106641;
  D.3407_969 = poissons_ratio_841 * coefficient_956;
  D.3408_970 = -D.3407_969;
  D.5048_106405 = (real8 *) ivtmp.923_106427;
  #   SMT.774_10649 = V_MAY_DEF <SMT.774_6041>;
  MEM[base: D.5048_106405, offset: -72B] = D.3408_970;
  D.5049_5994 = (real8 *) ivtmp.923_106427;
  #   SMT.774_10650 = V_MAY_DEF <SMT.774_10649>;
  MEM[base: D.5049_5994, offset: -40B] = D.3408_970;
  D.5050_6098 = (real8 *) ivtmp.923_106427;
  #   SMT.774_10651 = V_MAY_DEF <SMT.774_10650>;
  MEM[base: D.5050_6098, offset: -8B] = coefficient_956;
  D.5051_106649 = (<unnamed type>) n_lsm.835_6013;
  D.5052_106648 = (real8 *) D.5051_106649;
  D.5053_5998 = D.5052_106648 * -8B;
  D.5054_6102 = (real8 *) ivtmp.920_106432;
  D.5055_106892 = D.5053_5998 + D.5054_6102;
  D.5056_106891 = (int8) n_lsm.835_6013;
  D.5057_106890 = (real8 *) D.5056_106891;
  #   VUSE <SMT.774_10651>;
  D.3267_1009 = MEM[base: D.5055_106892, index: D.5057_106890, step: 8B,
offset: -8B];
  #   VUSE <SMT.774_10651>;
  radius_of_curvature.466_1010 = radius_of_curvature;
  D.3394_1011 = D.3267_1009 / radius_of_curvature.466_1010;
  #   VUSE <SMT.774_10651>;
  time.463_1015 = time;
  D.3397_1016 = time.463_1015 *
6.283185307000000108246240415610373020172119140625e+0;
  D.3398_1017 = D.3397_1016 * D.3396_951;
  D.3416_1018 = __builtin_cos (D.3398_1017);

which is because strain_tensor is allocated like

      allocate (strain_tensor(3,3,number_of_sample_points))

and the gfortran allocate does not have DECL_IS_MALLOC as it has the
not supported interface 'void allocate (void**, int)'.  Would we
be able to translate this 'allocate' call by using 'internal_malloc'
it would get a different aliasing tag and just work.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[rguenth at gcc dot gnu dot org]

------- Comment #15 from rguenth at gcc dot gnu dot org  2006-12-07 20:50 -------
Note that for _vectorization_ of sincos we need the following:

 - a vectorized sincos implementation and vectorization support for it
   (that's the easier part).  At the moment we only can vectorize calls
   to sqrt.
 - re-order the recip pass (tree-ssa-math-opts.c) and the loop
   passes.  At the moment we generate the sincos _after_ vectorization


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[dorit at il dot ibm dot com]

------- Comment #16 from dorit at il dot ibm dot com  2006-12-12 20:59 -------
(In reply to comment #13)
Looks like what's blocking vectorization of the loop is:

sinc.f90:8: note: value used after loop.
sinc.f90:8: note: not vectorized: relevant stmt not supported: D.1408_32 =
(*radius_31)[D.1407_30]

i.e., there is a value computed in the loop that is also used after the loop
(coefficient__lsm.61_26), and the above stmt is in its def-use chain, as can be
seen from the loop snippet below: 


  # n_3 = PHI <1(3), n_70(5)>;
<L14>:;
...
  D.1408_32 = (*radius_31)[D.1407_30];
...
  D.1410_35 = reciptmp.60_24 * D.1408_32;
...
  D.1419_63 = D.1410_35 * pretmp.53_51;
  coefficient__lsm.61_26 = D.1419_63;
...
  (*tmp_49)[D.1426_67] = D.1419_63;
  n_70 = n_3 + 1;
  if (n_3 == D.1398_5) goto <L20>; else goto <L19>;

<L19>:;
  goto <bb 4> (<L14>);

  # coefficient__lsm.61_54 = PHI <coefficient__lsm.61_26(4)>;
<L20>:;
  *coefficient_44 = coefficient__lsm.61_54;

<L10>:;
  return;


We currently support a computation that is used after the loop only if the
computation is a reduction. We have a patch in autovect branch that provides
the first step towards supporting this situation in general, but it needs more
work. How important is this feature do you think? 

In the meantime you can try to use a different variable for the coefficient
inside the loop, and after the loop read the desired value from memory to set
the coefficient function argument (hopefully this will disconnect the use
outside the loop from the def inside the loop).    


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[burnus at gcc dot gnu dot org]

------- Comment #17 from burnus at gcc dot gnu dot org  2006-12-17 23:19 -------
Using the patch of PR 30223 (add cbrt, cexpi and sincos to Fortran), I don't
get any speed up (21.95s for fatigue).

Using additionally http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00958.html
(folding of cexp ()) I get >20% speedup: 21.95s --> 17.851s

This is much less than the 28.3s -> 18.3s change at
http://www.suse.de/~gcctest/c++bench/polyhedron/polyhedron-summary.txt-2-0.html
which presumably means I miss at least one patch.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[rguenth at gcc dot gnu dot org]

------- Comment #18 from rguenth at gcc dot gnu dot org  2007-01-22 11:11 -------
Subject: Bug 30038

Author: rguenth
Date: Mon Jan 22 11:11:00 2007
New Revision: 121052

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=121052
Log:
2007-01-22  Richard Guenther  <rguenther@suse.de>

        PR tree-optimization/30038
        * tree-ssa-math-opts.c (maybe_record_sincos): New static helper
        function.
        (execute_cse_sincos_1): Likewise.
        (execute_cse_sincos): Likewise.
        (gate_cse_sincos): Likewise.
        (pass_cse_sincos): New pass CSEing sin() and cos() calls using
        the cexpi() canonicalization of sincos().
        * tree-pass.h (pass_cse_sincos): Declare.
        * passes.c (init_optimization_passes): New pass pas_cse_sincos.

        * gcc.dg/builtins-62.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/builtins-62.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/passes.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-pass.h
    trunk/gcc/tree-ssa-math-opts.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038

[Bug tree-optimization/30038] Call to sin(x), cos(x) should be transformed to sincos(x)

[rguenth at gcc dot gnu dot org]

------- Comment #19 from rguenth at gcc dot gnu dot org  2007-01-22 11:12 -------
Fixed.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.3.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30038