Fixing improper conversion from sin() to sinf() in optimization mode.

I have made a new patch according to your comments. I found several
references saying that the precision 2p+2 is OK for the sqrt
conversion (one here:
http://www.cs.berkeley.edu/~fateman/generic/algorithms.pdf). The new
patch is pasted as below.

Thank you for all the suggestions, Joseph!


Cong


       tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));

On Tue, Sep 3, 2013 at 3:38 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Tue, 3 Sep 2013, Cong Hou wrote:
>
>> Could you please tell me how to check the precision of long double in
>> GCC on different platforms?
>
> REAL_MODE_FORMAT (TYPE_MODE (long_double_type_node))->p
>
> (but you should be referring to the relevant types - "type", the type
> being converted to, "itype", the type of the function being called in the
> source code, "TREE_TYPE (arg0)", the type of the argument after extensions
> have been removed, and "newtype", computed from those - so you should have
> expressions like the above with two or more of those four types, but not
> with long_double_type_node directly).
>
> The patch submission will need to include a proper analysis to justify to
> the reader why the particular inequality with particular types from those
> four is correct in all cases where the relevant code may be executed.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

On Wed, 4 Sep 2013, Cong Hou wrote:

> I have made a new patch according to your comments. I found several
> references saying that the precision 2p+2 is OK for the sqrt
> conversion (one here:
> http://www.cs.berkeley.edu/~fateman/generic/algorithms.pdf). The new
> patch is pasted as below.

This patch submission still fails to pay attention to various of my 
comments.

On Wed, Sep 4, 2013 at 1:53 PM, Cong Hou <congh@google.com> wrote:
> I have made a new patch according to your comments. I found several
> references saying that the precision 2p+2 is OK for the sqrt
> conversion (one here:
> http://www.cs.berkeley.edu/~fateman/generic/algorithms.pdf). The new
> patch is pasted as below.
>
> Thank you for all the suggestions, Joseph!
>
>
> Cong
>
>
> Index: gcc/testsuite/gcc.c-torture/execute/20030125-1.c
> ===================================================================
> --- gcc/testsuite/gcc.c-torture/execute/20030125-1.c (revision 201891)
> +++ gcc/testsuite/gcc.c-torture/execute/20030125-1.c (working copy)
> @@ -44,11 +44,11 @@ __attribute__ ((noinline))
>  double
>  sin(double a)
>  {
> - abort ();
> + return a;
>  }
>  __attribute__ ((noinline))
>  float
>  sinf(float a)
>  {
> - return a;
> + abort ();
>  }
> Index: gcc/convert.c
> ===================================================================
> --- gcc/convert.c (revision 201891)
> +++ gcc/convert.c (working copy)
> @@ -135,16 +135,34 @@ convert_to_real (tree type, tree expr)
>    CASE_MATHFN (COS)
>    CASE_MATHFN (ERF)
>    CASE_MATHFN (ERFC)
> -  CASE_MATHFN (FABS)
>    CASE_MATHFN (LOG)
>    CASE_MATHFN (LOG10)
>    CASE_MATHFN (LOG2)
>    CASE_MATHFN (LOG1P)
> -  CASE_MATHFN (LOGB)
>    CASE_MATHFN (SIN)
> -  CASE_MATHFN (SQRT)
>    CASE_MATHFN (TAN)
>    CASE_MATHFN (TANH)
> +  CASE_MATHFN (SQRT)
> +
> +            /* The above functions (except sqrt) are not safe to do
> this conversion. */
> +            if (!flag_unsafe_math_optimizations)
> +            {
> +              /* sqrtl?(T1) could be safely converted into sqrtf?(T2) only if
> +               * p1 >= p2*2+2, where p1 and p2 are precisions of T1 and T2. */

Two spaces after T2.

Perhaps making the comment clearer?

                  it is safe to do the following:
                        float f1 = sqrt ((double) f2);
                         -->
                        float f1 = sqrtf (f2);

                 But conditionally safe for the following
                        double d1 = sqrtl ((long double) d2);
                          -->
                        double d1 = sqrt (d2);

                 depending on the precision of the long double type on
the target. ...< Add your
                 reference here.>


David

> +              if ((fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL))
> +              {

Fix indentation.

> +                int p1 = REAL_MODE_FORMAT (TYPE_MODE (type))->p;
> +                int p2 = (fcode == BUILT_IN_SQRTL) ?
> +                    REAL_MODE_FORMAT (TYPE_MODE (long_double_type_node))->p :
> +                    REAL_MODE_FORMAT (TYPE_MODE (double_type_node))->p;
> +                if (p2 < p1 * 2 + 2)
> +                  break;
> +              }
> +              else
> +                break;
> +            }
> +  CASE_MATHFN (FABS)
> +  CASE_MATHFN (LOGB)
>  #undef CASE_MATHFN
>      {
>        tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));
>
> On Tue, Sep 3, 2013 at 3:38 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>> On Tue, 3 Sep 2013, Cong Hou wrote:
>>
>>> Could you please tell me how to check the precision of long double in
>>> GCC on different platforms?
>>
>> REAL_MODE_FORMAT (TYPE_MODE (long_double_type_node))->p
>>
>> (but you should be referring to the relevant types - "type", the type
>> being converted to, "itype", the type of the function being called in the
>> source code, "TREE_TYPE (arg0)", the type of the argument after extensions
>> have been removed, and "newtype", computed from those - so you should have
>> expressions like the above with two or more of those four types, but not
>> with long_double_type_node directly).
>>
>> The patch submission will need to include a proper analysis to justify to
>> the reader why the particular inequality with particular types from those
>> four is correct in all cases where the relevant code may be executed.
>>
>> --
>> Joseph S. Myers
>> joseph@codesourcery.com

On Wed, Sep 4, 2013 at 1:59 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Wed, 4 Sep 2013, Cong Hou wrote:
>
>> I have made a new patch according to your comments. I found several
>> references saying that the precision 2p+2 is OK for the sqrt
>> conversion (one here:
>> http://www.cs.berkeley.edu/~fateman/generic/algorithms.pdf). The new
>> patch is pasted as below.
>
> This patch submission still fails to pay attention to various of my
> comments.
>

If you can provide inlined comments in the patch, that will be more
useful and productive.

thanks,

David


> --
> Joseph S. Myers
> joseph@codesourcery.com

On Wed, 4 Sep 2013, Xinliang David Li wrote:

> > This patch submission still fails to pay attention to various of my
> > comments.
> >
> 
> If you can provide inlined comments in the patch, that will be more
> useful and productive.

I have explained things several times in this thread already.  I see no 
point in repeating things when what I would say has already been said and 
ignored.  As far as I can tell, this latest patch submission has taken one 
line from the message it is in response to, and largely ignored the 
following two paragraphs (including where I explicitly say that said line 
should not appear literally in the source code at all).  But, repeating 
what I said before yet again:

  (but you should be referring to the relevant types

The patch does not do properly that.  It refers explicitly to 
long_double_type_node and double_type_node.

  - "type", the type 
  being converted to, "itype", the type of the function being called in the 
  source code, "TREE_TYPE (arg0)", the type of the argument after extensions 
  have been removed, and "newtype", computed from those

The patch only engages with "type".  I suspect "newtype" is what it really 
means there when using "type".  When it uses long_double_type_node and 
double_type_node, those should be "itype".

  - so you should have 
  expressions like the above with two or more of those four types, but not 
  with long_double_type_node directly).

See above.  The patch uses long_double_type_node directly, contrary to 
what I explicitly said.  You are free to disagree with something I say in 
a review - but in that case you need to reply specifically to the review 
comment explaining your rationale for disagreeing with it.  Just ignoring 
the comment and not mentioning the disagreement wastes the time of 
reviewers.

  The patch submission will need to include a proper analysis to justify to 
  the reader why the particular inequality with particular types from those 
  four is correct in all cases where the relevant code may be executed.

The comments only refer to "T1" and "T2" without explaining how they 
relate to the original expression (three types) or the variables within 
GCC dealing with various types (four types, because newtype gets 
involved).  I said back in 
<http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00161.html> and 
<http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01384.html> that three types 
are involved - when I say "the patch submission needs to include its own 
analysis for the full generality of three types", again, it's 
inappropriate for a patch to omit such an analysis without justification.  
The patch submission needs to include an analysis that properly explains 
the transformation involved and the conditions under which it is safe.  
Maybe starting along the lines of:

We have an expression of the form (T1) sqrtT2 ((T2) exprT3), where exprT3 
has type T3 (TREE_TYPE (ARG0)), T2 is the type of the floating-point 
square root function being used (ITYPE), T1 is TYPE and all these types 
are binary floating-point types.  We wish to optimize if possible to an 
expression of the form (T1) sqrtT4 ((T4) exprT3), where T4 (NEWTYPE) is 
narrower than T2.  (Then explain the choice of T4 and the conditions under 
which the transformation is safe, with appropriate references.)

I suggest that for the next patch submission you (the patch submitter) 
make sure you spend at least an hour properly understanding the issues and 
all the previous messages in the thread and writing up the detailed, 
coherent explanation of the transformation and analysis of the issues that 
I asked for some time ago, as if for a reader who hasn't read any of this 
thread or looked at this transformation in GCC before.  I've certainly 
spent longer than that on review in this thread.  It's normal for a patch 
involving anything at all tricky to take hours to write up (and also 
normal for one to discover, in the course of writing the detailed coherent 
analysis for people who aren't familiar with the code and the issues 
involved, that there's in fact something wrong with the patch and it needs 
revisiting before submission).