This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]