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

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

[Cong Hou]

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

When a sin() (cos(), log(), etc.) function is called on a value of
float type and the returned double value is converted to another value
of float type, GCC converts this function call into a float version
(sinf()) in the optimization mode. This avoids two type conversions
and the float version function call usually takes less time. However,
this can result in different result and therefore is unsafe.

For example, the following code produces different results using -O0
(correct), but the same results using -Ox other than -O0 (incorrect).


#include <stdio.h>
#include <math.h>

int main()
{
  float v = 1;
  printf("%.20f\n", (float)sin(v));
  printf("%.20f\n", sinf(v));
}


In this patch, we do this conversion only when the flag
-funsafe-math-optimizations is set. The patch is shown below.


thanks,

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)
@@ -99,7 +99,7 @@ convert_to_real (tree type, tree expr)
   /* Disable until we figure out how to decide whether the functions are
      present in runtime.  */
   /* Convert (float)sqrt((double)x) where x is float into sqrtf(x) */
-  if (optimize
+  if (optimize && flag_unsafe_math_optimizations
       && (TYPE_MODE (type) == TYPE_MODE (double_type_node)
           || TYPE_MODE (type) == TYPE_MODE (float_type_node)))
     {

[-- Attachment #2: diff.txt --]
[-- Type: text/plain, Size: 1030 bytes --]

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)
@@ -99,7 +99,7 @@ convert_to_real (tree type, tree expr)
   /* Disable until we figure out how to decide whether the functions are
      present in runtime.  */
   /* Convert (float)sqrt((double)x) where x is float into sqrtf(x) */
-  if (optimize
+  if (optimize && flag_unsafe_math_optimizations
       && (TYPE_MODE (type) == TYPE_MODE (double_type_node)
           || TYPE_MODE (type) == TYPE_MODE (float_type_node)))
     {

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Joseph S. Myers]

On Tue, 20 Aug 2013, Cong Hou wrote:

> When a sin() (cos(), log(), etc.) function is called on a value of
> float type and the returned double value is converted to another value
> of float type, GCC converts this function call into a float version
> (sinf()) in the optimization mode. This avoids two type conversions
> and the float version function call usually takes less time. However,
> this can result in different result and therefore is unsafe.

Whether it's safe depends on the existence of input values for which the 
double-rounded result is different from the single-rounded result.  It's 
certainly safe for some of the functions for which the optimization is 
enabled, regardless of the precisions involved (fabs, logb).  For sqrt, if 
the smaller precision is p then the larger precision being at least 2p+3 
(I think) makes things same.  For the other cases, exhaustive searches may 
be needed to determine when the conversion is safe.

(Actually, this code appears to deal with cases with up to three types 
involved - the operand, the result and the function that's called.  This 
complicates the analysis a bit, but the same principles apply.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Joseph S. Myers]

On Fri, 30 Aug 2013, Cong Hou wrote:

> I have done a simple experiment and found that it may only be safe to do
> this conversion for fabs() and sqrt(). For fabs() it is easy to see the
> safety. For sqrt(), I tried many random floating point values on two
> versions and did not see a difference. Although it cannot prove its safety,
> my proposed patch (shown below) does change the situation.
> 
> However, for other functions, my experiment shows it is unsafe to do this
> conversion. Those functions are:
> 
> sin, cos, sinh, cosh, asin, acos, asinh, acosh, tan, tanh, atan, atanh,
> log, log10, log1p, log2, logb, cbrt, erf, erfc, exp, exp2, expm1.

I don't see why it would be unsafe for logb - can you give an example 
(exact float input value as hex float, and the values you believe logb 
should return for float and double).

For sqrt you appear to have ignored my explanation of the necessary and 
sufficient condition on the precisions of the respective types.  The 
conversion is not safe for sqrt if the two types are double and long 
double and long double is x86 extended, for example.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Joseph S. Myers]

On Sat, 31 Aug 2013, Cong Hou wrote:

> > I don't see why it would be unsafe for logb - can you give an example
> > (exact float input value as hex float, and the values you believe logb
> > should return for float and double).
> >
> 
> Please try the following code (you will get different results whether to
> use optimization mode):
> 
> #include <math.h>
> #include <stdio.h>
> 
> int main()
> {
>   int i = 0x3edc67d5;
>   float f = *((float*)&i);
>   float r1 = logb(f);
>   float r2 = logbf(f);
>   printf("%x %x\n", *((int*)&r1), *((int*)&r2));
> }

(a) Please stop sending HTML email, so your messages reach the mailing 
list, and resend your messages so far to the list.  The mailing list needs 
to see the whole of both sides of the discussion of any patch being 
proposed for GCC.

(b) I referred to the values *you believe logb should return*.  
Optimization is not meant to preserve library bugs; the comparison should 
be on the basis of correctly rounded results from both float and double 
functions.  The correct return from logb appears to be -2 here, and I get 
that from both logb and logbf with current git glibc.  The existence of a 
bug in some old library is not relevant here.

(c) I always advise writing such tests as *valid C code* using hex floats 
(or if really necessary, unions), not *invalid C code* with aliasing 
violations.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

I have fixed my test code and replaced those aliasing violations with
unions. Now the test result shows logb() is safe for the conversions.

The conclusion is, logb() and fabs() are always safe for the
converion, and sqrt() is unsafe for the conversion from sqrtl(double)
to sqrt(double). Other math functions are not safe for the conversion.

The new test code I used is shown below:

#include <time.h>
#include <stdlib.h>
#include <math.h>
#include <stdio.h>

typedef union
{
    int i;
    float f;
} T32;

typedef union
{
    long long int i;
    double f;
} T64;

#define N 10000000

#define test_math_func(func) \
for (i = 0; i < N; ++i) \
{ \
  int d = rand(), e = rand(); \
  if (d == 0) continue; \
  T32 v, r1, r2; \
  v.f = (float)e / d; \
  r1.f = func(v.f), r2.f = func##f(v.f); \
  if (r1.f != r2.f) \
  { \
    printf("%s double -> float (%X) %X %X\n", #func, v.i, r1.i, r2.i); \
    break; \
  } \
} \
for (i = 0; i < N; ++i) \
{ \
  int d = rand(), e = rand(); \
  if (d == 0) continue; \
  T32 v, r1, r2; \
  v.f = (float)e / d; \
  r1.f = func##l(v.f), r2.f = func##f(v.f); \
  if (r1.f != r2.f) \
  { \
    printf("%s long double -> float (%X) %X %X\n", #func, v.i, r1.i, r2.i); \
    break; \
  } \
} \
for (i = 0; i < N; ++i) \
{ \
  int d = rand(), e = rand(); \
  if (d == 0) continue; \
  T64 v, r1, r2; \
  v.f = (double)e / d; \
  r1.f = func##l(v.f), r2.f = func(v.f); \
  if (r1.f != r2.f) \
  { \
    printf("%s long double -> double (%016llX) %016llX %016llX\n",
#func, v.i, r1.i, r2.i); \
    break; \
  } \
}

int main()
{
  int i;
  test_math_func(sin);
  test_math_func(cos);
  test_math_func(sinh);
  test_math_func(cosh);
  test_math_func(asin);
  test_math_func(acos);
  test_math_func(asinh);
  test_math_func(acosh);
  test_math_func(tan);
  test_math_func(tanh);
  test_math_func(atan);
  test_math_func(atanh);
  test_math_func(log);
  test_math_func(log10);
  test_math_func(log1p);
  test_math_func(log2);
  test_math_func(logb);
  test_math_func(cbrt);
  test_math_func(erf);
  test_math_func(erfc);
  test_math_func(exp);
  test_math_func(exp2);
  test_math_func(expm1);
  test_math_func(sqrt);
  test_math_func(fabs);
}


I have modified the patch according to this new conclusion. The patch
is pasted as below.

thanks,
Cong


===================================================================
--- gcc/convert.c (revision 201891)
+++ gcc/convert.c (working copy)
@@ -135,16 +135,24 @@ 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)
+            /* The above functions are not safe to do this conversion. */
+            if (!flag_unsafe_math_optimizations)
+              break;
+  CASE_MATHFN (SQRT)
+            /* sqrtl(double) cannot be safely converted to sqrt(double). */
+            if (fcode == BUILT_IN_SQRTL &&
+                (TYPE_MODE (type) == TYPE_MODE (double_type_node)) &&
+                !flag_unsafe_math_optimizations)
+              break;
+  CASE_MATHFN (FABS)
+  CASE_MATHFN (LOGB)
 #undef CASE_MATHFN
     {
       tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));
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 ();
 }

On Sat, Aug 31, 2013 at 9:24 AM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Sat, 31 Aug 2013, Cong Hou wrote:
>
>> > I don't see why it would be unsafe for logb - can you give an example
>> > (exact float input value as hex float, and the values you believe logb
>> > should return for float and double).
>> >
>>
>> Please try the following code (you will get different results whether to
>> use optimization mode):
>>
>> #include <math.h>
>> #include <stdio.h>
>>
>> int main()
>> {
>>   int i = 0x3edc67d5;
>>   float f = *((float*)&i);
>>   float r1 = logb(f);
>>   float r2 = logbf(f);
>>   printf("%x %x\n", *((int*)&r1), *((int*)&r2));
>> }
>
> (a) Please stop sending HTML email, so your messages reach the mailing
> list, and resend your messages so far to the list.  The mailing list needs
> to see the whole of both sides of the discussion of any patch being
> proposed for GCC.
>
> (b) I referred to the values *you believe logb should return*.
> Optimization is not meant to preserve library bugs; the comparison should
> be on the basis of correctly rounded results from both float and double
> functions.  The correct return from logb appears to be -2 here, and I get
> that from both logb and logbf with current git glibc.  The existence of a
> bug in some old library is not relevant here.
>
> (c) I always advise writing such tests as *valid C code* using hex floats
> (or if really necessary, unions), not *invalid C code* with aliasing
> violations.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Joseph S. Myers]

On Tue, 3 Sep 2013, Cong Hou wrote:

> +  CASE_MATHFN (SQRT)
> +            /* sqrtl(double) cannot be safely converted to sqrt(double). */
> +            if (fcode == BUILT_IN_SQRTL &&
> +                (TYPE_MODE (type) == TYPE_MODE (double_type_node)) &&
> +                !flag_unsafe_math_optimizations)
> +              break;

Please reread my previous messages on this subject and try again, with 
regard to both the patch itself and the accompanying analysis.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Xinliang David Li]

From Joseph:

"The
conversion is not safe for sqrt if the two types are double and long
double and long double is x86 extended, for example."

This is not reflected in the patch.

David


On Tue, Sep 3, 2013 at 2:27 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Tue, 3 Sep 2013, Cong Hou wrote:
>
>> +  CASE_MATHFN (SQRT)
>> +            /* sqrtl(double) cannot be safely converted to sqrt(double). */
>> +            if (fcode == BUILT_IN_SQRTL &&
>> +                (TYPE_MODE (type) == TYPE_MODE (double_type_node)) &&
>> +                !flag_unsafe_math_optimizations)
>> +              break;
>
> Please reread my previous messages on this subject and try again, with
> regard to both the patch itself and the accompanying analysis.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Joseph S. Myers]

On Tue, 3 Sep 2013, Xinliang David Li wrote:

> >From Joseph:
> 
> "The
> conversion is not safe for sqrt if the two types are double and long
> double and long double is x86 extended, for example."
> 
> This is not reflected in the patch.

No, the problem is that it tries to reflect it but hardcodes the specific 
example I gave, rather than following the logic I explained regarding the 
precisions of the types involved, which depend on the target.  And since I 
only gave a simplified analysis, for two types when this function deals 
with cases involving three types, the patch submission needs to include 
its own analysis for the full generality of three types to justify the 
logic used (as inequalities involving the three precisions).  (I suspect 
it reduces to the case of two types so you don't need to go into the 
details of reasoning about floating point to produce the more general 
analysis.  But in any case, it's for the patch submitter to give the full 
explanation.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

Could you please tell me how to check the precision of long double in
GCC on different platforms?

Thank you!


Cong

On Tue, Sep 3, 2013 at 2:43 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Tue, 3 Sep 2013, Xinliang David Li wrote:
>
>> >From Joseph:
>>
>> "The
>> conversion is not safe for sqrt if the two types are double and long
>> double and long double is x86 extended, for example."
>>
>> This is not reflected in the patch.
>
> No, the problem is that it tries to reflect it but hardcodes the specific
> example I gave, rather than following the logic I explained regarding the
> precisions of the types involved, which depend on the target.  And since I
> only gave a simplified analysis, for two types when this function deals
> with cases involving three types, the patch submission needs to include
> its own analysis for the full generality of three types to justify the
> logic used (as inequalities involving the three precisions).  (I suspect
> it reduces to the case of two types so you don't need to go into the
> details of reasoning about floating point to produce the more general
> analysis.  But in any case, it's for the patch submitter to give the full
> explanation.)
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Joseph S. Myers]

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

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

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. */
+              if ((fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL))
+              {
+                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

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Joseph S. Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Xinliang David Li]

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

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

Updated patch according to your comment (tabs are not pasted here).

Cong


Index: gcc/convert.c
===================================================================
--- gcc/convert.c (revision 201891)
+++ gcc/convert.c (working copy)
@@ -135,16 +135,40 @@ 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.
+   For example, on x86 the conversion from float(sqrt((double)f) to
+   sqrtf(f) is safe where f has the type float, since float has 23 bits
+   precision and double has 52 bits precision, and 52 > 23*2+2.
+   However, the conversion from double(sqrtl((long double)d) to
+   sqrt(d) is unsafe where d has the type double. This is because
+   long double has 63 bits precision and then 63 < 52*2+2.  */
+ if ((fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL))
+  {
+    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));
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 ();
 }

On Wed, Sep 4, 2013 at 2:21 PM, Xinliang David Li <davidxl@google.com> wrote:
> 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

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Joseph S. Myers]

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).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

First, thank you for your detailed comments again! Then I deeply
apologize for not explaining my patch properly and responding to your
previous comment. I didn't understand thoroughly the problem before
submitting the patch.

Previously I only considered the following three conversions for sqrt():


1: (float) sqrt ((double) float_val)  ->  sqrtf (float_val)
2: (float) sqrtl ((long double) float_val)  ->  sqrtf (float_val)
3: (double) sqrtl ((long double) double_val)  ->  sqrt (double_val)


We have four types here:

TYPE is the type to which the result of the function call is converted.
ITYPE is the type of the math call expression.
TREE_TYPE(arg0) is the type of the function argument (before type conversion).
NEWTYPE is chosen from TYPE and TREE_TYPE(arg0) with higher precision.
It will be the type of the new math call expression after conversion.

For all three cases above, TYPE is always the same as NEWTYPE. That is
why I only considered TYPE during the precision comparison. ITYPE can
only be double_type_node or long_double_type_node depending on the
type of the math function. That is why I explicitly used those two
types instead of ITYPE (no correctness issue). But you are right,
ITYPE is more elegant and better here.

After further analysis, I found I missed two more cases. Note that we
have the following conditions according to the code in convert.c:

TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TYPE)
TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TREE_TYPE(arg0))
TYPE_PRECISION (NEWTYPE) < TYPE_PRECISION (ITYPE)

the last condition comes from the fact that we only consider
converting a math function call into another one with narrower type.
Therefore we have

TYPE_PRECISION(TYPE) < TYPE_PRECISION (ITYPE)
TYPE_PRECISION(TREE_TYPE(arg0)) < TYPE_PRECISION (ITYPE)

So for sqrt(), TYPE and TREE_TYPE(arg0) can only be float, and for
sqrtl(), TYPE and TREE_TYPE(arg0) can be either float or double with
four possible combinations. Therefore we have two more conversions to
consider besides the three ones I mentioned above:


4: (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)
5: (double) sqrtl ((long double) float_val)  ->  sqrt ((double) float_val)


For the first conversion here, TYPE (float) is different from NEWTYPE
(double), and my previous patch doesn't handle this case.The correct
way is to compare precisions of ITYPE and NEWTYPE now.

To sum up, we are converting the expression

(TYPE) sqrtITYPE ((ITYPE) expr)

to

(TYPE) sqrtNEWTYPE ((NEWTYPE) expr)

and we require

PRECISION (ITYPE) >= PRECISION (NEWTYPE) * 2 + 2

to make it a safe conversion.


The new patch is pasted below.

I appreciate your detailed comments and analysis, and next time when I
submit a patch I will be more carefully about the reviewer's comment.


Thank you!

Cong



Index: gcc/convert.c
===================================================================
--- gcc/convert.c (revision 201891)
+++ gcc/convert.c (working copy)
@@ -135,16 +135,19 @@ 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)
+    /* The above functions are not safe to do this conversion. */
+    if (!flag_unsafe_math_optimizations)
+      break;
+  CASE_MATHFN (SQRT)
+  CASE_MATHFN (FABS)
+  CASE_MATHFN (LOGB)
 #undef CASE_MATHFN
     {
       tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));
@@ -155,6 +158,27 @@ convert_to_real (tree type, tree expr)
       if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
  newtype = TREE_TYPE (arg0);

+      /* We consider to convert
+
+     (T1) sqrtT2 ((T2) exprT3)
+ to
+     (T1) sqrtT4 ((T4) exprT3)
+
+  , where T1 is TYPE, T2 is ITYPE, T3 is TREE_TYPE (ARG0),
+ and T4 is NEWTYPE. All those types are of floating point types.
+ T4 (NEWTYPE) should be narrower than T2 (ITYPE). This conversion
+ is safe only if P1 >= P2*2+2, where P1 and P2 are precisions of
+ T2 and T4. See the following URL for a reference:
+ http://stackoverflow.com/questions/9235456/determining-floating-point-square-root
+ */
+      if (fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL)
+ {
+  int p1 = REAL_MODE_FORMAT (TYPE_MODE (itype))->p;
+  int p2 = REAL_MODE_FORMAT (TYPE_MODE (newtype))->p;
+  if (p1 < p2 * 2 + 2 && !flag_unsafe_math_optimizations)
+    break;
+ }
+
       /* Be careful about integer to fp conversions.
  These may overflow still.  */
       if (FLOAT_TYPE_P (TREE_TYPE (arg0))
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 ();
 }

On Wed, Sep 4, 2013 at 3:26 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> 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).
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Xinliang David Li]

On Fri, Sep 6, 2013 at 3:24 PM, Cong Hou <congh@google.com> wrote:
> First, thank you for your detailed comments again! Then I deeply
> apologize for not explaining my patch properly and responding to your
> previous comment. I didn't understand thoroughly the problem before
> submitting the patch.
>
> Previously I only considered the following three conversions for sqrt():
>
>
> 1: (float) sqrt ((double) float_val)  ->  sqrtf (float_val)
> 2: (float) sqrtl ((long double) float_val)  ->  sqrtf (float_val)
> 3: (double) sqrtl ((long double) double_val)  ->  sqrt (double_val)
>
>
> We have four types here:
>
> TYPE is the type to which the result of the function call is converted.
> ITYPE is the type of the math call expression.
> TREE_TYPE(arg0) is the type of the function argument (before type conversion).
> NEWTYPE is chosen from TYPE and TREE_TYPE(arg0) with higher precision.
> It will be the type of the new math call expression after conversion.
>
> For all three cases above, TYPE is always the same as NEWTYPE. That is
> why I only considered TYPE during the precision comparison. ITYPE can
> only be double_type_node or long_double_type_node depending on the
> type of the math function. That is why I explicitly used those two
> types instead of ITYPE (no correctness issue). But you are right,
> ITYPE is more elegant and better here.
>
> After further analysis, I found I missed two more cases. Note that we
> have the following conditions according to the code in convert.c:
>
> TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TYPE)
> TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TREE_TYPE(arg0))
> TYPE_PRECISION (NEWTYPE) < TYPE_PRECISION (ITYPE)
>
> the last condition comes from the fact that we only consider
> converting a math function call into another one with narrower type.
> Therefore we have
>
> TYPE_PRECISION(TYPE) < TYPE_PRECISION (ITYPE)
> TYPE_PRECISION(TREE_TYPE(arg0)) < TYPE_PRECISION (ITYPE)
>
> So for sqrt(), TYPE and TREE_TYPE(arg0) can only be float, and for
> sqrtl(), TYPE and TREE_TYPE(arg0) can be either float or double with
> four possible combinations. Therefore we have two more conversions to
> consider besides the three ones I mentioned above:
>
>
> 4: (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)
> 5: (double) sqrtl ((long double) float_val)  ->  sqrt ((double) float_val)
>
>
> For the first conversion here, TYPE (float) is different from NEWTYPE
> (double), and my previous patch doesn't handle this case.The correct
> way is to compare precisions of ITYPE and NEWTYPE now.
>
> To sum up, we are converting the expression
>
> (TYPE) sqrtITYPE ((ITYPE) expr)
>
> to
>
> (TYPE) sqrtNEWTYPE ((NEWTYPE) expr)
>
> and we require
>
> PRECISION (ITYPE) >= PRECISION (NEWTYPE) * 2 + 2
>
> to make it a safe conversion.
>
>
> The new patch is pasted below.
>
> I appreciate your detailed comments and analysis, and next time when I
> submit a patch I will be more carefully about the reviewer's comment.
>
>
> Thank you!
>
> Cong
>
>
>
> Index: gcc/convert.c
> ===================================================================
> --- gcc/convert.c (revision 201891)
> +++ gcc/convert.c (working copy)
> @@ -135,16 +135,19 @@ 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)
> +    /* The above functions are not safe to do this conversion. */
> +    if (!flag_unsafe_math_optimizations)
> +      break;
> +  CASE_MATHFN (SQRT)
> +  CASE_MATHFN (FABS)
> +  CASE_MATHFN (LOGB)
>  #undef CASE_MATHFN
>      {
>        tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));
> @@ -155,6 +158,27 @@ convert_to_real (tree type, tree expr)
>        if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
>   newtype = TREE_TYPE (arg0);
>
> +      /* We consider to convert
> +
> +     (T1) sqrtT2 ((T2) exprT3)
> + to
> +     (T1) sqrtT4 ((T4) exprT3)

Should this be

          (T4) sqrtT4 ((T4) exprT3)
> +
> +  , where T1 is TYPE, T2 is ITYPE, T3 is TREE_TYPE (ARG0),
> + and T4 is NEWTYPE.

NEWTYPE is also the wider one between T1 and T3.

David

> All those types are of floating point types.
> + T4 (NEWTYPE) should be narrower than T2 (ITYPE). This conversion
> + is safe only if P1 >= P2*2+2, where P1 and P2 are precisions of
> + T2 and T4. See the following URL for a reference:
> + http://stackoverflow.com/questions/9235456/determining-floating-point-square-root
> + */
> +      if (fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL)
> + {
> +  int p1 = REAL_MODE_FORMAT (TYPE_MODE (itype))->p;
> +  int p2 = REAL_MODE_FORMAT (TYPE_MODE (newtype))->p;
> +  if (p1 < p2 * 2 + 2 && !flag_unsafe_math_optimizations)
> +    break;
> + }
> +
>        /* Be careful about integer to fp conversions.
>   These may overflow still.  */
>        if (FLOAT_TYPE_P (TREE_TYPE (arg0))
> 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 ();
>  }
>
> On Wed, Sep 4, 2013 at 3:26 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>> 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).
>>
>> --
>> Joseph S. Myers
>> joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

On Mon, Sep 9, 2013 at 6:26 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Fri, Sep 6, 2013 at 3:24 PM, Cong Hou <congh@google.com> wrote:
>> First, thank you for your detailed comments again! Then I deeply
>> apologize for not explaining my patch properly and responding to your
>> previous comment. I didn't understand thoroughly the problem before
>> submitting the patch.
>>
>> Previously I only considered the following three conversions for sqrt():
>>
>>
>> 1: (float) sqrt ((double) float_val)  ->  sqrtf (float_val)
>> 2: (float) sqrtl ((long double) float_val)  ->  sqrtf (float_val)
>> 3: (double) sqrtl ((long double) double_val)  ->  sqrt (double_val)
>>
>>
>> We have four types here:
>>
>> TYPE is the type to which the result of the function call is converted.
>> ITYPE is the type of the math call expression.
>> TREE_TYPE(arg0) is the type of the function argument (before type conversion).
>> NEWTYPE is chosen from TYPE and TREE_TYPE(arg0) with higher precision.
>> It will be the type of the new math call expression after conversion.
>>
>> For all three cases above, TYPE is always the same as NEWTYPE. That is
>> why I only considered TYPE during the precision comparison. ITYPE can
>> only be double_type_node or long_double_type_node depending on the
>> type of the math function. That is why I explicitly used those two
>> types instead of ITYPE (no correctness issue). But you are right,
>> ITYPE is more elegant and better here.
>>
>> After further analysis, I found I missed two more cases. Note that we
>> have the following conditions according to the code in convert.c:
>>
>> TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TYPE)
>> TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TREE_TYPE(arg0))
>> TYPE_PRECISION (NEWTYPE) < TYPE_PRECISION (ITYPE)
>>
>> the last condition comes from the fact that we only consider
>> converting a math function call into another one with narrower type.
>> Therefore we have
>>
>> TYPE_PRECISION(TYPE) < TYPE_PRECISION (ITYPE)
>> TYPE_PRECISION(TREE_TYPE(arg0)) < TYPE_PRECISION (ITYPE)
>>
>> So for sqrt(), TYPE and TREE_TYPE(arg0) can only be float, and for
>> sqrtl(), TYPE and TREE_TYPE(arg0) can be either float or double with
>> four possible combinations. Therefore we have two more conversions to
>> consider besides the three ones I mentioned above:
>>
>>
>> 4: (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)
>> 5: (double) sqrtl ((long double) float_val)  ->  sqrt ((double) float_val)
>>
>>
>> For the first conversion here, TYPE (float) is different from NEWTYPE
>> (double), and my previous patch doesn't handle this case.The correct
>> way is to compare precisions of ITYPE and NEWTYPE now.
>>
>> To sum up, we are converting the expression
>>
>> (TYPE) sqrtITYPE ((ITYPE) expr)
>>
>> to
>>
>> (TYPE) sqrtNEWTYPE ((NEWTYPE) expr)
>>
>> and we require
>>
>> PRECISION (ITYPE) >= PRECISION (NEWTYPE) * 2 + 2
>>
>> to make it a safe conversion.
>>
>>
>> The new patch is pasted below.
>>
>> I appreciate your detailed comments and analysis, and next time when I
>> submit a patch I will be more carefully about the reviewer's comment.
>>
>>
>> Thank you!
>>
>> Cong
>>
>>
>>
>> Index: gcc/convert.c
>> ===================================================================
>> --- gcc/convert.c (revision 201891)
>> +++ gcc/convert.c (working copy)
>> @@ -135,16 +135,19 @@ 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)
>> +    /* The above functions are not safe to do this conversion. */
>> +    if (!flag_unsafe_math_optimizations)
>> +      break;
>> +  CASE_MATHFN (SQRT)
>> +  CASE_MATHFN (FABS)
>> +  CASE_MATHFN (LOGB)
>>  #undef CASE_MATHFN
>>      {
>>        tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));
>> @@ -155,6 +158,27 @@ convert_to_real (tree type, tree expr)
>>        if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
>>   newtype = TREE_TYPE (arg0);
>>
>> +      /* We consider to convert
>> +
>> +     (T1) sqrtT2 ((T2) exprT3)
>> + to
>> +     (T1) sqrtT4 ((T4) exprT3)
>
> Should this be
>
>           (T4) sqrtT4 ((T4) exprT3)

T4 is not necessarily the same as T1. For the conversion:

 (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)

T4 is double and T1 is float.


>> +
>> +  , where T1 is TYPE, T2 is ITYPE, T3 is TREE_TYPE (ARG0),
>> + and T4 is NEWTYPE.
>
> NEWTYPE is also the wider one between T1 and T3.

Right. Actually this is easy to catch from the context just before
this comment.

tree newtype = type;
if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
    newtype = TREE_TYPE (arg0);



thanks,
Cong


>
> David
>
>> All those types are of floating point types.
>> + T4 (NEWTYPE) should be narrower than T2 (ITYPE). This conversion
>> + is safe only if P1 >= P2*2+2, where P1 and P2 are precisions of
>> + T2 and T4. See the following URL for a reference:
>> + http://stackoverflow.com/questions/9235456/determining-floating-point-square-root
>> + */
>> +      if (fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL)
>> + {
>> +  int p1 = REAL_MODE_FORMAT (TYPE_MODE (itype))->p;
>> +  int p2 = REAL_MODE_FORMAT (TYPE_MODE (newtype))->p;
>> +  if (p1 < p2 * 2 + 2 && !flag_unsafe_math_optimizations)
>> +    break;
>> + }
>> +
>>        /* Be careful about integer to fp conversions.
>>   These may overflow still.  */
>>        if (FLOAT_TYPE_P (TREE_TYPE (arg0))
>> 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 ();
>>  }
>>
>> On Wed, Sep 4, 2013 at 3:26 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>>> 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).
>>>
>>> --
>>> Joseph S. Myers
>>> joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

Any comment or more suggestions on this patch?


thanks,
Cong

On Mon, Sep 9, 2013 at 7:28 PM, Cong Hou <congh@google.com> wrote:
> On Mon, Sep 9, 2013 at 6:26 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Fri, Sep 6, 2013 at 3:24 PM, Cong Hou <congh@google.com> wrote:
>>> First, thank you for your detailed comments again! Then I deeply
>>> apologize for not explaining my patch properly and responding to your
>>> previous comment. I didn't understand thoroughly the problem before
>>> submitting the patch.
>>>
>>> Previously I only considered the following three conversions for sqrt():
>>>
>>>
>>> 1: (float) sqrt ((double) float_val)  ->  sqrtf (float_val)
>>> 2: (float) sqrtl ((long double) float_val)  ->  sqrtf (float_val)
>>> 3: (double) sqrtl ((long double) double_val)  ->  sqrt (double_val)
>>>
>>>
>>> We have four types here:
>>>
>>> TYPE is the type to which the result of the function call is converted.
>>> ITYPE is the type of the math call expression.
>>> TREE_TYPE(arg0) is the type of the function argument (before type conversion).
>>> NEWTYPE is chosen from TYPE and TREE_TYPE(arg0) with higher precision.
>>> It will be the type of the new math call expression after conversion.
>>>
>>> For all three cases above, TYPE is always the same as NEWTYPE. That is
>>> why I only considered TYPE during the precision comparison. ITYPE can
>>> only be double_type_node or long_double_type_node depending on the
>>> type of the math function. That is why I explicitly used those two
>>> types instead of ITYPE (no correctness issue). But you are right,
>>> ITYPE is more elegant and better here.
>>>
>>> After further analysis, I found I missed two more cases. Note that we
>>> have the following conditions according to the code in convert.c:
>>>
>>> TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TYPE)
>>> TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TREE_TYPE(arg0))
>>> TYPE_PRECISION (NEWTYPE) < TYPE_PRECISION (ITYPE)
>>>
>>> the last condition comes from the fact that we only consider
>>> converting a math function call into another one with narrower type.
>>> Therefore we have
>>>
>>> TYPE_PRECISION(TYPE) < TYPE_PRECISION (ITYPE)
>>> TYPE_PRECISION(TREE_TYPE(arg0)) < TYPE_PRECISION (ITYPE)
>>>
>>> So for sqrt(), TYPE and TREE_TYPE(arg0) can only be float, and for
>>> sqrtl(), TYPE and TREE_TYPE(arg0) can be either float or double with
>>> four possible combinations. Therefore we have two more conversions to
>>> consider besides the three ones I mentioned above:
>>>
>>>
>>> 4: (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)
>>> 5: (double) sqrtl ((long double) float_val)  ->  sqrt ((double) float_val)
>>>
>>>
>>> For the first conversion here, TYPE (float) is different from NEWTYPE
>>> (double), and my previous patch doesn't handle this case.The correct
>>> way is to compare precisions of ITYPE and NEWTYPE now.
>>>
>>> To sum up, we are converting the expression
>>>
>>> (TYPE) sqrtITYPE ((ITYPE) expr)
>>>
>>> to
>>>
>>> (TYPE) sqrtNEWTYPE ((NEWTYPE) expr)
>>>
>>> and we require
>>>
>>> PRECISION (ITYPE) >= PRECISION (NEWTYPE) * 2 + 2
>>>
>>> to make it a safe conversion.
>>>
>>>
>>> The new patch is pasted below.
>>>
>>> I appreciate your detailed comments and analysis, and next time when I
>>> submit a patch I will be more carefully about the reviewer's comment.
>>>
>>>
>>> Thank you!
>>>
>>> Cong
>>>
>>>
>>>
>>> Index: gcc/convert.c
>>> ===================================================================
>>> --- gcc/convert.c (revision 201891)
>>> +++ gcc/convert.c (working copy)
>>> @@ -135,16 +135,19 @@ 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)
>>> +    /* The above functions are not safe to do this conversion. */
>>> +    if (!flag_unsafe_math_optimizations)
>>> +      break;
>>> +  CASE_MATHFN (SQRT)
>>> +  CASE_MATHFN (FABS)
>>> +  CASE_MATHFN (LOGB)
>>>  #undef CASE_MATHFN
>>>      {
>>>        tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));
>>> @@ -155,6 +158,27 @@ convert_to_real (tree type, tree expr)
>>>        if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
>>>   newtype = TREE_TYPE (arg0);
>>>
>>> +      /* We consider to convert
>>> +
>>> +     (T1) sqrtT2 ((T2) exprT3)
>>> + to
>>> +     (T1) sqrtT4 ((T4) exprT3)
>>
>> Should this be
>>
>>           (T4) sqrtT4 ((T4) exprT3)
>
> T4 is not necessarily the same as T1. For the conversion:
>
>  (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)
>
> T4 is double and T1 is float.
>
>
>>> +
>>> +  , where T1 is TYPE, T2 is ITYPE, T3 is TREE_TYPE (ARG0),
>>> + and T4 is NEWTYPE.
>>
>> NEWTYPE is also the wider one between T1 and T3.
>
> Right. Actually this is easy to catch from the context just before
> this comment.
>
> tree newtype = type;
> if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
>     newtype = TREE_TYPE (arg0);
>
>
>
> thanks,
> Cong
>
>
>>
>> David
>>
>>> All those types are of floating point types.
>>> + T4 (NEWTYPE) should be narrower than T2 (ITYPE). This conversion
>>> + is safe only if P1 >= P2*2+2, where P1 and P2 are precisions of
>>> + T2 and T4. See the following URL for a reference:
>>> + http://stackoverflow.com/questions/9235456/determining-floating-point-square-root
>>> + */
>>> +      if (fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL)
>>> + {
>>> +  int p1 = REAL_MODE_FORMAT (TYPE_MODE (itype))->p;
>>> +  int p2 = REAL_MODE_FORMAT (TYPE_MODE (newtype))->p;
>>> +  if (p1 < p2 * 2 + 2 && !flag_unsafe_math_optimizations)
>>> +    break;
>>> + }
>>> +
>>>        /* Be careful about integer to fp conversions.
>>>   These may overflow still.  */
>>>        if (FLOAT_TYPE_P (TREE_TYPE (arg0))
>>> 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 ();
>>>  }
>>>
>>> On Wed, Sep 4, 2013 at 3:26 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>>>> 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).
>>>>
>>>> --
>>>> Joseph S. Myers
>>>> joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

Ping...


thanks,
Cong


On Fri, Sep 20, 2013 at 9:49 AM, Cong Hou <congh@google.com> wrote:
> Any comment or more suggestions on this patch?
>
>
> thanks,
> Cong
>
> On Mon, Sep 9, 2013 at 7:28 PM, Cong Hou <congh@google.com> wrote:
>> On Mon, Sep 9, 2013 at 6:26 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Fri, Sep 6, 2013 at 3:24 PM, Cong Hou <congh@google.com> wrote:
>>>> First, thank you for your detailed comments again! Then I deeply
>>>> apologize for not explaining my patch properly and responding to your
>>>> previous comment. I didn't understand thoroughly the problem before
>>>> submitting the patch.
>>>>
>>>> Previously I only considered the following three conversions for sqrt():
>>>>
>>>>
>>>> 1: (float) sqrt ((double) float_val)  ->  sqrtf (float_val)
>>>> 2: (float) sqrtl ((long double) float_val)  ->  sqrtf (float_val)
>>>> 3: (double) sqrtl ((long double) double_val)  ->  sqrt (double_val)
>>>>
>>>>
>>>> We have four types here:
>>>>
>>>> TYPE is the type to which the result of the function call is converted.
>>>> ITYPE is the type of the math call expression.
>>>> TREE_TYPE(arg0) is the type of the function argument (before type conversion).
>>>> NEWTYPE is chosen from TYPE and TREE_TYPE(arg0) with higher precision.
>>>> It will be the type of the new math call expression after conversion.
>>>>
>>>> For all three cases above, TYPE is always the same as NEWTYPE. That is
>>>> why I only considered TYPE during the precision comparison. ITYPE can
>>>> only be double_type_node or long_double_type_node depending on the
>>>> type of the math function. That is why I explicitly used those two
>>>> types instead of ITYPE (no correctness issue). But you are right,
>>>> ITYPE is more elegant and better here.
>>>>
>>>> After further analysis, I found I missed two more cases. Note that we
>>>> have the following conditions according to the code in convert.c:
>>>>
>>>> TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TYPE)
>>>> TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TREE_TYPE(arg0))
>>>> TYPE_PRECISION (NEWTYPE) < TYPE_PRECISION (ITYPE)
>>>>
>>>> the last condition comes from the fact that we only consider
>>>> converting a math function call into another one with narrower type.
>>>> Therefore we have
>>>>
>>>> TYPE_PRECISION(TYPE) < TYPE_PRECISION (ITYPE)
>>>> TYPE_PRECISION(TREE_TYPE(arg0)) < TYPE_PRECISION (ITYPE)
>>>>
>>>> So for sqrt(), TYPE and TREE_TYPE(arg0) can only be float, and for
>>>> sqrtl(), TYPE and TREE_TYPE(arg0) can be either float or double with
>>>> four possible combinations. Therefore we have two more conversions to
>>>> consider besides the three ones I mentioned above:
>>>>
>>>>
>>>> 4: (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)
>>>> 5: (double) sqrtl ((long double) float_val)  ->  sqrt ((double) float_val)
>>>>
>>>>
>>>> For the first conversion here, TYPE (float) is different from NEWTYPE
>>>> (double), and my previous patch doesn't handle this case.The correct
>>>> way is to compare precisions of ITYPE and NEWTYPE now.
>>>>
>>>> To sum up, we are converting the expression
>>>>
>>>> (TYPE) sqrtITYPE ((ITYPE) expr)
>>>>
>>>> to
>>>>
>>>> (TYPE) sqrtNEWTYPE ((NEWTYPE) expr)
>>>>
>>>> and we require
>>>>
>>>> PRECISION (ITYPE) >= PRECISION (NEWTYPE) * 2 + 2
>>>>
>>>> to make it a safe conversion.
>>>>
>>>>
>>>> The new patch is pasted below.
>>>>
>>>> I appreciate your detailed comments and analysis, and next time when I
>>>> submit a patch I will be more carefully about the reviewer's comment.
>>>>
>>>>
>>>> Thank you!
>>>>
>>>> Cong
>>>>
>>>>
>>>>
>>>> Index: gcc/convert.c
>>>> ===================================================================
>>>> --- gcc/convert.c (revision 201891)
>>>> +++ gcc/convert.c (working copy)
>>>> @@ -135,16 +135,19 @@ 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)
>>>> +    /* The above functions are not safe to do this conversion. */
>>>> +    if (!flag_unsafe_math_optimizations)
>>>> +      break;
>>>> +  CASE_MATHFN (SQRT)
>>>> +  CASE_MATHFN (FABS)
>>>> +  CASE_MATHFN (LOGB)
>>>>  #undef CASE_MATHFN
>>>>      {
>>>>        tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));
>>>> @@ -155,6 +158,27 @@ convert_to_real (tree type, tree expr)
>>>>        if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
>>>>   newtype = TREE_TYPE (arg0);
>>>>
>>>> +      /* We consider to convert
>>>> +
>>>> +     (T1) sqrtT2 ((T2) exprT3)
>>>> + to
>>>> +     (T1) sqrtT4 ((T4) exprT3)
>>>
>>> Should this be
>>>
>>>           (T4) sqrtT4 ((T4) exprT3)
>>
>> T4 is not necessarily the same as T1. For the conversion:
>>
>>  (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)
>>
>> T4 is double and T1 is float.
>>
>>
>>>> +
>>>> +  , where T1 is TYPE, T2 is ITYPE, T3 is TREE_TYPE (ARG0),
>>>> + and T4 is NEWTYPE.
>>>
>>> NEWTYPE is also the wider one between T1 and T3.
>>
>> Right. Actually this is easy to catch from the context just before
>> this comment.
>>
>> tree newtype = type;
>> if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
>>     newtype = TREE_TYPE (arg0);
>>
>>
>>
>> thanks,
>> Cong
>>
>>
>>>
>>> David
>>>
>>>> All those types are of floating point types.
>>>> + T4 (NEWTYPE) should be narrower than T2 (ITYPE). This conversion
>>>> + is safe only if P1 >= P2*2+2, where P1 and P2 are precisions of
>>>> + T2 and T4. See the following URL for a reference:
>>>> + http://stackoverflow.com/questions/9235456/determining-floating-point-square-root
>>>> + */
>>>> +      if (fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL)
>>>> + {
>>>> +  int p1 = REAL_MODE_FORMAT (TYPE_MODE (itype))->p;
>>>> +  int p2 = REAL_MODE_FORMAT (TYPE_MODE (newtype))->p;
>>>> +  if (p1 < p2 * 2 + 2 && !flag_unsafe_math_optimizations)
>>>> +    break;
>>>> + }
>>>> +
>>>>        /* Be careful about integer to fp conversions.
>>>>   These may overflow still.  */
>>>>        if (FLOAT_TYPE_P (TREE_TYPE (arg0))
>>>> 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 ();
>>>>  }
>>>>
>>>> On Wed, Sep 4, 2013 at 3:26 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>>>>> 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).
>>>>>
>>>>> --
>>>>> Joseph S. Myers
>>>>> joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Joseph S. Myers]

On Fri, 6 Sep 2013, Cong Hou wrote:

> 4: (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)

I don't believe this case is in fact safe even if precision (long double) 
>= precision (double) * 2 + 2 (when your patch would allow it).

The result that precision (double) * 2 + 2 is sufficient for the result of 
rounding the long double value to double to be the same as the result of 
rounding once from infinite precision to double would I think also mean 
the same when rounding of the infinite-precision result to float happens 
once - that is, if instead of (float) sqrt (double_val) you have fsqrt 
(double_val) (fsqrt being the proposed function in draft TS 18661-1 for 
computing a square root of a double value, rounded exactly once to float).  
But here the question isn't whether rounding to long double then float 
gives the same result as rounding to float.  It's whether rounding to long 
double then float gives the same result as rounding to double then float.

Consider, for example, double_val = 0x1.0000020000011p+0.  The square root 
rounded once to float (and so the result if you go via long double and 
long double is sufficiently precise) is 0x1.000002p+0.  But the square 
root rounded to double is 0x1.000001p+0, which on rounding to float 
produces 0x1p+0.

> 5: (double) sqrtl ((long double) float_val)  ->  sqrt ((double) float_val)

(This case, however, *is* safe if long double is sufficiently precise; the 
conversion of float to long double is trivially equivalent to a conversion 
involving an intermediate "double" type, so it reduces to a case for which 
the standard formula involving precisions of just two types applies.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

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

You are right. I am not an expert on numerical analysis, but I tested
your case and it proves the number 4 conversion is not safe.

Now we have four conversions which are safe once the precision
requirement is satisfied. I added a condition if (type != newtype) to
remove the unsafe one, as in this case once more conversion is added
which leads to the unsafe issue. If you think this condition does not
make sense please let me know.

The new patch is shown below (the attached file has tabs).

Thank you very much!



thanks,
Cong



Index: gcc/convert.c
===================================================================
--- gcc/convert.c (revision 203250)
+++ gcc/convert.c (working copy)
@@ -135,16 +135,19 @@ 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)
+    /* The above functions are not safe to do this conversion.  */
+    if (!flag_unsafe_math_optimizations)
+      break;
+  CASE_MATHFN (SQRT)
+  CASE_MATHFN (FABS)
+  CASE_MATHFN (LOGB)
 #undef CASE_MATHFN
     {
       tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));
@@ -155,13 +158,43 @@ convert_to_real (tree type, tree expr)
       if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
  newtype = TREE_TYPE (arg0);

+      /* We consider to convert
+
+     (T1) sqrtT2 ((T2) exprT3)
+ to
+     (T1) sqrtT4 ((T4) exprT3)
+
+  , where T1 is TYPE, T2 is ITYPE, T3 is TREE_TYPE (ARG0),
+ and T4 is NEWTYPE. All those types are of floating point types.
+ T4 (NEWTYPE) should be narrower than T2 (ITYPE). This conversion
+ is safe only if P1 >= P2*2+2, where P1 and P2 are precisions of
+ T2 and T4. See the following URL for a reference:
+ http://stackoverflow.com/questions/9235456/determining-floating-point-square-root
+ */
+      if ((fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL)
+  && !flag_unsafe_math_optimizations)
+ {
+  /* The following conversion is unsafe even the precision condition
+     below is satisfied:
+
+     (float) sqrtl ((long double) double_val) -> (float) sqrt (double_val)
+    */
+  if (type != newtype)
+    break;
+
+  int p1 = REAL_MODE_FORMAT (TYPE_MODE (itype))->p;
+  int p2 = REAL_MODE_FORMAT (TYPE_MODE (newtype))->p;
+  if (p1 < p2 * 2 + 2)
+    break;
+ }
+
       /* Be careful about integer to fp conversions.
  These may overflow still.  */
       if (FLOAT_TYPE_P (TREE_TYPE (arg0))
   && TYPE_PRECISION (newtype) < TYPE_PRECISION (itype)
   && (TYPE_MODE (newtype) == TYPE_MODE (double_type_node)
       || TYPE_MODE (newtype) == TYPE_MODE (float_type_node)))
-        {
+ {
   tree fn = mathfn_built_in (newtype, fcode);

   if (fn)
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog (revision 203250)
+++ gcc/ChangeLog (working copy)
@@ -1,3 +1,9 @@
+2013-10-07  Cong Hou  <congh@google.com>
+
+ * convert.c (convert_to_real): Forbid unsafe math function
+ conversions including sin/cos/log etc. Add precision check
+ for sqrt.
+
 2013-10-07  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

  * config/rs6000/rs6000.c (altivec_expand_vec_perm_const_le): New.
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog (revision 203250)
+++ gcc/testsuite/ChangeLog (working copy)
@@ -1,3 +1,7 @@
+2013-10-07  Cong Hou  <congh@google.com>
+
+ * gcc.c-torture/execute/20030125-1.c: Update.
+
 2013-10-07  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

  * gcc.target/powerpc/pr43154.c: Skip for ppc64 little endian.
Index: gcc/testsuite/gcc.c-torture/execute/20030125-1.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/20030125-1.c (revision 203250)
+++ 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 ();
 }




On Thu, Oct 3, 2013 at 5:06 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Fri, 6 Sep 2013, Cong Hou wrote:
>
>> 4: (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)
>
> I don't believe this case is in fact safe even if precision (long double)
>>= precision (double) * 2 + 2 (when your patch would allow it).
>
> The result that precision (double) * 2 + 2 is sufficient for the result of
> rounding the long double value to double to be the same as the result of
> rounding once from infinite precision to double would I think also mean
> the same when rounding of the infinite-precision result to float happens
> once - that is, if instead of (float) sqrt (double_val) you have fsqrt
> (double_val) (fsqrt being the proposed function in draft TS 18661-1 for
> computing a square root of a double value, rounded exactly once to float).
> But here the question isn't whether rounding to long double then float
> gives the same result as rounding to float.  It's whether rounding to long
> double then float gives the same result as rounding to double then float.
>
> Consider, for example, double_val = 0x1.0000020000011p+0.  The square root
> rounded once to float (and so the result if you go via long double and
> long double is sufficiently precise) is 0x1.000002p+0.  But the square
> root rounded to double is 0x1.000001p+0, which on rounding to float
> produces 0x1p+0.
>
>> 5: (double) sqrtl ((long double) float_val)  ->  sqrt ((double) float_val)
>
> (This case, however, *is* safe if long double is sufficiently precise; the
> conversion of float to long double is trivially equivalent to a conversion
> involving an intermediate "double" type, so it reduces to a case for which
> the standard formula involving precisions of just two types applies.)
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3771 bytes --]

Index: gcc/convert.c
===================================================================
--- gcc/convert.c	(revision 203250)
+++ gcc/convert.c	(working copy)
@@ -135,16 +135,19 @@ 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)
+	    /* The above functions are not safe to do this conversion.  */
+	    if (!flag_unsafe_math_optimizations)
+	      break;
+ 	  CASE_MATHFN (SQRT)
+ 	  CASE_MATHFN (FABS)
+ 	  CASE_MATHFN (LOGB)
 #undef CASE_MATHFN
 	    {
 	      tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));
@@ -155,13 +158,43 @@ convert_to_real (tree type, tree expr)
 	      if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
 		newtype = TREE_TYPE (arg0);
 
+	      /* We consider to convert 
+
+		     (T1) sqrtT2 ((T2) exprT3) 
+		 to 
+		     (T1) sqrtT4 ((T4) exprT3)
+
+		  , where T1 is TYPE, T2 is ITYPE, T3 is TREE_TYPE (ARG0), 
+		 and T4 is NEWTYPE. All those types are of floating point types.
+		 T4 (NEWTYPE) should be narrower than T2 (ITYPE). This conversion
+		 is safe only if P1 >= P2*2+2, where P1 and P2 are precisions of
+		 T2 and T4. See the following URL for a reference:
+		 http://stackoverflow.com/questions/9235456/determining-floating-point-square-root
+		 */ 
+	      if ((fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL)
+		  && !flag_unsafe_math_optimizations)
+		{
+		  /* The following conversion is unsafe even the precision condition
+		     below is satisfied:
+		     
+		     (float) sqrtl ((long double) double_val) -> (float) sqrt (double_val)
+		    */
+		  if (type != newtype)
+		    break;
+
+		  int p1 = REAL_MODE_FORMAT (TYPE_MODE (itype))->p;
+		  int p2 = REAL_MODE_FORMAT (TYPE_MODE (newtype))->p;
+		  if (p1 < p2 * 2 + 2)
+		    break;
+		}
+
 	      /* Be careful about integer to fp conversions.
 		 These may overflow still.  */
 	      if (FLOAT_TYPE_P (TREE_TYPE (arg0))
 		  && TYPE_PRECISION (newtype) < TYPE_PRECISION (itype)
 		  && (TYPE_MODE (newtype) == TYPE_MODE (double_type_node)
 		      || TYPE_MODE (newtype) == TYPE_MODE (float_type_node)))
-	        {
+		{
 		  tree fn = mathfn_built_in (newtype, fcode);
 
 		  if (fn)
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 203250)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2013-10-07  Cong Hou  <congh@google.com>
+
+	* convert.c (convert_to_real): Forbid unsafe math function
+	conversions including sin/cos/log etc. Add precision check 
+	for sqrt.
+
 2013-10-07  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
 
 	* config/rs6000/rs6000.c (altivec_expand_vec_perm_const_le): New.
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 203250)
+++ gcc/testsuite/ChangeLog	(working copy)
@@ -1,3 +1,7 @@
+2013-10-07  Cong Hou  <congh@google.com>
+
+	* gcc.c-torture/execute/20030125-1.c: Update. 
+
 2013-10-07  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
 
 	* gcc.target/powerpc/pr43154.c: Skip for ppc64 little endian.
Index: gcc/testsuite/gcc.c-torture/execute/20030125-1.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/20030125-1.c	(revision 203250)
+++ 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 ();
 }

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

Ping?


thanks,
Cong


On Mon, Oct 7, 2013 at 10:15 AM, Cong Hou <congh@google.com> wrote:
> You are right. I am not an expert on numerical analysis, but I tested
> your case and it proves the number 4 conversion is not safe.
>
> Now we have four conversions which are safe once the precision
> requirement is satisfied. I added a condition if (type != newtype) to
> remove the unsafe one, as in this case once more conversion is added
> which leads to the unsafe issue. If you think this condition does not
> make sense please let me know.
>
> The new patch is shown below (the attached file has tabs).
>
> Thank you very much!
>
>
>
> thanks,
> Cong
>
>
>
> Index: gcc/convert.c
> ===================================================================
> --- gcc/convert.c (revision 203250)
> +++ gcc/convert.c (working copy)
> @@ -135,16 +135,19 @@ 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)
> +    /* The above functions are not safe to do this conversion.  */
> +    if (!flag_unsafe_math_optimizations)
> +      break;
> +  CASE_MATHFN (SQRT)
> +  CASE_MATHFN (FABS)
> +  CASE_MATHFN (LOGB)
>  #undef CASE_MATHFN
>      {
>        tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0));
> @@ -155,13 +158,43 @@ convert_to_real (tree type, tree expr)
>        if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type))
>   newtype = TREE_TYPE (arg0);
>
> +      /* We consider to convert
> +
> +     (T1) sqrtT2 ((T2) exprT3)
> + to
> +     (T1) sqrtT4 ((T4) exprT3)
> +
> +  , where T1 is TYPE, T2 is ITYPE, T3 is TREE_TYPE (ARG0),
> + and T4 is NEWTYPE. All those types are of floating point types.
> + T4 (NEWTYPE) should be narrower than T2 (ITYPE). This conversion
> + is safe only if P1 >= P2*2+2, where P1 and P2 are precisions of
> + T2 and T4. See the following URL for a reference:
> + http://stackoverflow.com/questions/9235456/determining-floating-point-square-root
> + */
> +      if ((fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL)
> +  && !flag_unsafe_math_optimizations)
> + {
> +  /* The following conversion is unsafe even the precision condition
> +     below is satisfied:
> +
> +     (float) sqrtl ((long double) double_val) -> (float) sqrt (double_val)
> +    */
> +  if (type != newtype)
> +    break;
> +
> +  int p1 = REAL_MODE_FORMAT (TYPE_MODE (itype))->p;
> +  int p2 = REAL_MODE_FORMAT (TYPE_MODE (newtype))->p;
> +  if (p1 < p2 * 2 + 2)
> +    break;
> + }
> +
>        /* Be careful about integer to fp conversions.
>   These may overflow still.  */
>        if (FLOAT_TYPE_P (TREE_TYPE (arg0))
>    && TYPE_PRECISION (newtype) < TYPE_PRECISION (itype)
>    && (TYPE_MODE (newtype) == TYPE_MODE (double_type_node)
>        || TYPE_MODE (newtype) == TYPE_MODE (float_type_node)))
> -        {
> + {
>    tree fn = mathfn_built_in (newtype, fcode);
>
>    if (fn)
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog (revision 203250)
> +++ gcc/ChangeLog (working copy)
> @@ -1,3 +1,9 @@
> +2013-10-07  Cong Hou  <congh@google.com>
> +
> + * convert.c (convert_to_real): Forbid unsafe math function
> + conversions including sin/cos/log etc. Add precision check
> + for sqrt.
> +
>  2013-10-07  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
>
>   * config/rs6000/rs6000.c (altivec_expand_vec_perm_const_le): New.
> Index: gcc/testsuite/ChangeLog
> ===================================================================
> --- gcc/testsuite/ChangeLog (revision 203250)
> +++ gcc/testsuite/ChangeLog (working copy)
> @@ -1,3 +1,7 @@
> +2013-10-07  Cong Hou  <congh@google.com>
> +
> + * gcc.c-torture/execute/20030125-1.c: Update.
> +
>  2013-10-07  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
>
>   * gcc.target/powerpc/pr43154.c: Skip for ppc64 little endian.
> Index: gcc/testsuite/gcc.c-torture/execute/20030125-1.c
> ===================================================================
> --- gcc/testsuite/gcc.c-torture/execute/20030125-1.c (revision 203250)
> +++ 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 ();
>  }
>
>
>
>
> On Thu, Oct 3, 2013 at 5:06 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>> On Fri, 6 Sep 2013, Cong Hou wrote:
>>
>>> 4: (float) sqrtl ((long double) double_val)  ->  (float) sqrt (double_val)
>>
>> I don't believe this case is in fact safe even if precision (long double)
>>>= precision (double) * 2 + 2 (when your patch would allow it).
>>
>> The result that precision (double) * 2 + 2 is sufficient for the result of
>> rounding the long double value to double to be the same as the result of
>> rounding once from infinite precision to double would I think also mean
>> the same when rounding of the infinite-precision result to float happens
>> once - that is, if instead of (float) sqrt (double_val) you have fsqrt
>> (double_val) (fsqrt being the proposed function in draft TS 18661-1 for
>> computing a square root of a double value, rounded exactly once to float).
>> But here the question isn't whether rounding to long double then float
>> gives the same result as rounding to float.  It's whether rounding to long
>> double then float gives the same result as rounding to double then float.
>>
>> Consider, for example, double_val = 0x1.0000020000011p+0.  The square root
>> rounded once to float (and so the result if you go via long double and
>> long double is sufficiently precise) is 0x1.000002p+0.  But the square
>> root rounded to double is 0x1.000001p+0, which on rounding to float
>> produces 0x1p+0.
>>
>>> 5: (double) sqrtl ((long double) float_val)  ->  sqrt ((double) float_val)
>>
>> (This case, however, *is* safe if long double is sufficiently precise; the
>> conversion of float to long double is trivially equivalent to a conversion
>> involving an intermediate "double" type, so it reduces to a case for which
>> the standard formula involving precisions of just two types applies.)
>>
>> --
>> Joseph S. Myers
>> joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Joseph S. Myers]

On Mon, 7 Oct 2013, Cong Hou wrote:

> +  if (type != newtype)
> +    break;

That comparison would wrongly treat as different cases where the types 
differ only in one being a typedef, having qualifiers, etc. - or if in 
future GCC implemented proposed TS 18661-3, cases where they differ in 
e.g. one being float and the other _Float32 (defined as distinct types 
that are not compatible although they have the same representation and 
alignment).  I think the right test here, bearing in mind the _Float32 
case where types may not be compatible, is TYPE_MODE (type) != TYPE_MODE 
(newtype) - if the types have the same mode, they have the same set of 
values and so are not different in any way that matters for this 
optimization.  OK with that change.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Cong Hou]

I have updated the patch according to your suggestion, and have
committed the patch as the bootstrapping and make check both get
passed.

Thank you for your patient help on this patch! I learned a lot from it.


thanks,
Cong


On Wed, Oct 23, 2013 at 1:13 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Mon, 7 Oct 2013, Cong Hou wrote:
>
>> +  if (type != newtype)
>> +    break;
>
> That comparison would wrongly treat as different cases where the types
> differ only in one being a typedef, having qualifiers, etc. - or if in
> future GCC implemented proposed TS 18661-3, cases where they differ in
> e.g. one being float and the other _Float32 (defined as distinct types
> that are not compatible although they have the same representation and
> alignment).  I think the right test here, bearing in mind the _Float32
> case where types may not be compatible, is TYPE_MODE (type) != TYPE_MODE
> (newtype) - if the types have the same mode, they have the same set of
> values and so are not different in any way that matters for this
> optimization.  OK with that change.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Xinliang David Li]

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

Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.

[Bernhard Reutner-Fischer]

On 4 September 2013 00:17:00 Cong Hou <congh@google.com> wrote:
> Could you please tell me how to check the precision of long double in
> GCC on different platforms?

I did not follow your discussion but..
http://uclibc.org/~aldot/precision_check.f

Or something along those lines in your favourite language.
HTH..
> Thank you!
>
>
> Cong


Sent with AquaMail for Android
http://www.aqua-mail.com