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

[Cong Hou <congh at google dot com>]

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: patch.txt
Description: Text document