* [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
@ 2013-08-20 23:19 Cong Hou
2013-08-23 21:04 ` Joseph S. Myers
0 siblings, 1 reply; 27+ messages in thread
From: Cong Hou @ 2013-08-20 23:19 UTC (permalink / raw)
To: gcc-patches; +Cc: David Li
[-- 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)))
{
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-08-20 23:19 [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode Cong Hou
@ 2013-08-23 21:04 ` Joseph S. Myers
[not found] ` <CAK=A3=3=gLhTso3+AF-BmiONPsEpP3dGTFtOAZPbh+oteYPTNA@mail.gmail.com>
0 siblings, 1 reply; 27+ messages in thread
From: Joseph S. Myers @ 2013-08-23 21:04 UTC (permalink / raw)
To: Cong Hou; +Cc: gcc-patches, David Li
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
[not found] ` <CAK=A3=3=gLhTso3+AF-BmiONPsEpP3dGTFtOAZPbh+oteYPTNA@mail.gmail.com>
@ 2013-08-30 21:53 ` Joseph S. Myers
[not found] ` <CAK=A3=0bQkcvprFZTtuJ0ZNbknSJixhMP559tiF3FFUL0zkmfw@mail.gmail.com>
0 siblings, 1 reply; 27+ messages in thread
From: Joseph S. Myers @ 2013-08-30 21:53 UTC (permalink / raw)
To: Cong Hou; +Cc: gcc-patches, David Li
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
[not found] ` <CAK=A3=0bQkcvprFZTtuJ0ZNbknSJixhMP559tiF3FFUL0zkmfw@mail.gmail.com>
@ 2013-08-31 17:14 ` Joseph S. Myers
2013-09-03 20:59 ` Cong Hou
0 siblings, 1 reply; 27+ messages in thread
From: Joseph S. Myers @ 2013-08-31 17:14 UTC (permalink / raw)
To: Cong Hou; +Cc: gcc-patches, David Li
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-08-31 17:14 ` Joseph S. Myers
@ 2013-09-03 20:59 ` Cong Hou
2013-09-03 21:27 ` Joseph S. Myers
0 siblings, 1 reply; 27+ messages in thread
From: Cong Hou @ 2013-09-03 20:59 UTC (permalink / raw)
To: Joseph S. Myers; +Cc: gcc-patches, David Li
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-03 20:59 ` Cong Hou
@ 2013-09-03 21:27 ` Joseph S. Myers
2013-09-03 21:31 ` Xinliang David Li
0 siblings, 1 reply; 27+ messages in thread
From: Joseph S. Myers @ 2013-09-03 21:27 UTC (permalink / raw)
To: Cong Hou; +Cc: gcc-patches, David Li
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-03 21:27 ` Joseph S. Myers
@ 2013-09-03 21:31 ` Xinliang David Li
2013-09-03 21:43 ` Joseph S. Myers
0 siblings, 1 reply; 27+ messages in thread
From: Xinliang David Li @ 2013-09-03 21:31 UTC (permalink / raw)
To: Joseph S. Myers; +Cc: Cong Hou, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-03 21:31 ` Xinliang David Li
@ 2013-09-03 21:43 ` Joseph S. Myers
2013-09-03 22:17 ` Cong Hou
0 siblings, 1 reply; 27+ messages in thread
From: Joseph S. Myers @ 2013-09-03 21:43 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Cong Hou, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-03 21:43 ` Joseph S. Myers
@ 2013-09-03 22:17 ` Cong Hou
2013-09-03 22:38 ` Joseph S. Myers
2013-09-03 22:53 ` Bernhard Reutner-Fischer
0 siblings, 2 replies; 27+ messages in thread
From: Cong Hou @ 2013-09-03 22:17 UTC (permalink / raw)
To: Joseph S. Myers; +Cc: Xinliang David Li, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-03 22:17 ` Cong Hou
@ 2013-09-03 22:38 ` Joseph S. Myers
2013-09-04 20:53 ` Cong Hou
2013-09-03 22:53 ` Bernhard Reutner-Fischer
1 sibling, 1 reply; 27+ messages in thread
From: Joseph S. Myers @ 2013-09-03 22:38 UTC (permalink / raw)
To: Cong Hou; +Cc: Xinliang David Li, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-03 22:17 ` Cong Hou
2013-09-03 22:38 ` Joseph S. Myers
@ 2013-09-03 22:53 ` Bernhard Reutner-Fischer
1 sibling, 0 replies; 27+ messages in thread
From: Bernhard Reutner-Fischer @ 2013-09-03 22:53 UTC (permalink / raw)
To: Cong Hou, Joseph S. Myers; +Cc: Xinliang David Li, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-03 22:38 ` Joseph S. Myers
@ 2013-09-04 20:53 ` Cong Hou
2013-09-04 20:59 ` Joseph S. Myers
2013-09-04 21:18 ` Xinliang David Li
0 siblings, 2 replies; 27+ messages in thread
From: Cong Hou @ 2013-09-04 20:53 UTC (permalink / raw)
To: Joseph S. Myers; +Cc: Xinliang David Li, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-04 20:53 ` Cong Hou
@ 2013-09-04 20:59 ` Joseph S. Myers
2013-09-04 21:21 ` Xinliang David Li
2013-09-04 21:18 ` Xinliang David Li
1 sibling, 1 reply; 27+ messages in thread
From: Joseph S. Myers @ 2013-09-04 20:59 UTC (permalink / raw)
To: Cong Hou; +Cc: Xinliang David Li, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-04 20:53 ` Cong Hou
2013-09-04 20:59 ` Joseph S. Myers
@ 2013-09-04 21:18 ` Xinliang David Li
1 sibling, 0 replies; 27+ messages in thread
From: Xinliang David Li @ 2013-09-04 21:18 UTC (permalink / raw)
To: Cong Hou; +Cc: Joseph S. Myers, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-04 20:59 ` Joseph S. Myers
@ 2013-09-04 21:21 ` Xinliang David Li
2013-09-04 21:48 ` Cong Hou
2013-09-04 22:26 ` Joseph S. Myers
0 siblings, 2 replies; 27+ messages in thread
From: Xinliang David Li @ 2013-09-04 21:21 UTC (permalink / raw)
To: Joseph S. Myers; +Cc: Cong Hou, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-04 21:21 ` Xinliang David Li
@ 2013-09-04 21:48 ` Cong Hou
2013-09-04 22:26 ` Joseph S. Myers
1 sibling, 0 replies; 27+ messages in thread
From: Cong Hou @ 2013-09-04 21:48 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Joseph S. Myers, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-04 21:21 ` Xinliang David Li
2013-09-04 21:48 ` Cong Hou
@ 2013-09-04 22:26 ` Joseph S. Myers
2013-09-06 22:24 ` Cong Hou
1 sibling, 1 reply; 27+ messages in thread
From: Joseph S. Myers @ 2013-09-04 22:26 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Cong Hou, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-04 22:26 ` Joseph S. Myers
@ 2013-09-06 22:24 ` Cong Hou
2013-09-10 2:20 ` Xinliang David Li
2013-10-04 0:06 ` Joseph S. Myers
0 siblings, 2 replies; 27+ messages in thread
From: Cong Hou @ 2013-09-06 22:24 UTC (permalink / raw)
To: Joseph S. Myers; +Cc: Xinliang David Li, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-06 22:24 ` Cong Hou
@ 2013-09-10 2:20 ` Xinliang David Li
2013-09-10 4:02 ` Cong Hou
2013-10-04 0:06 ` Joseph S. Myers
1 sibling, 1 reply; 27+ messages in thread
From: Xinliang David Li @ 2013-09-10 2:20 UTC (permalink / raw)
To: Cong Hou; +Cc: Joseph S. Myers, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-10 2:20 ` Xinliang David Li
@ 2013-09-10 4:02 ` Cong Hou
2013-09-20 17:05 ` Cong Hou
0 siblings, 1 reply; 27+ messages in thread
From: Cong Hou @ 2013-09-10 4:02 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Joseph S. Myers, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-10 4:02 ` Cong Hou
@ 2013-09-20 17:05 ` Cong Hou
2013-10-03 23:19 ` Cong Hou
0 siblings, 1 reply; 27+ messages in thread
From: Cong Hou @ 2013-09-20 17:05 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Joseph S. Myers, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-20 17:05 ` Cong Hou
@ 2013-10-03 23:19 ` Cong Hou
0 siblings, 0 replies; 27+ messages in thread
From: Cong Hou @ 2013-10-03 23:19 UTC (permalink / raw)
To: GCC Patches, Joseph S. Myers
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-09-06 22:24 ` Cong Hou
2013-09-10 2:20 ` Xinliang David Li
@ 2013-10-04 0:06 ` Joseph S. Myers
2013-10-07 17:15 ` Cong Hou
1 sibling, 1 reply; 27+ messages in thread
From: Joseph S. Myers @ 2013-10-04 0:06 UTC (permalink / raw)
To: Cong Hou; +Cc: Xinliang David Li, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-10-04 0:06 ` Joseph S. Myers
@ 2013-10-07 17:15 ` Cong Hou
2013-10-17 18:30 ` Cong Hou
2013-10-23 21:13 ` Joseph S. Myers
0 siblings, 2 replies; 27+ messages in thread
From: Cong Hou @ 2013-10-07 17:15 UTC (permalink / raw)
To: Joseph S. Myers, GCC Patches; +Cc: David Li
[-- 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 ();
}
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-10-07 17:15 ` Cong Hou
@ 2013-10-17 18:30 ` Cong Hou
2013-10-23 21:13 ` Joseph S. Myers
1 sibling, 0 replies; 27+ messages in thread
From: Cong Hou @ 2013-10-17 18:30 UTC (permalink / raw)
To: Joseph S. Myers, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-10-07 17:15 ` Cong Hou
2013-10-17 18:30 ` Cong Hou
@ 2013-10-23 21:13 ` Joseph S. Myers
2013-10-24 19:10 ` Cong Hou
1 sibling, 1 reply; 27+ messages in thread
From: Joseph S. Myers @ 2013-10-23 21:13 UTC (permalink / raw)
To: Cong Hou; +Cc: GCC Patches, David Li
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
2013-10-23 21:13 ` Joseph S. Myers
@ 2013-10-24 19:10 ` Cong Hou
0 siblings, 0 replies; 27+ messages in thread
From: Cong Hou @ 2013-10-24 19:10 UTC (permalink / raw)
To: Joseph S. Myers; +Cc: GCC Patches, David Li
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
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2013-10-24 18:23 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-20 23:19 [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode Cong Hou
2013-08-23 21:04 ` Joseph S. Myers
[not found] ` <CAK=A3=3=gLhTso3+AF-BmiONPsEpP3dGTFtOAZPbh+oteYPTNA@mail.gmail.com>
2013-08-30 21:53 ` Joseph S. Myers
[not found] ` <CAK=A3=0bQkcvprFZTtuJ0ZNbknSJixhMP559tiF3FFUL0zkmfw@mail.gmail.com>
2013-08-31 17:14 ` Joseph S. Myers
2013-09-03 20:59 ` Cong Hou
2013-09-03 21:27 ` Joseph S. Myers
2013-09-03 21:31 ` Xinliang David Li
2013-09-03 21:43 ` Joseph S. Myers
2013-09-03 22:17 ` Cong Hou
2013-09-03 22:38 ` Joseph S. Myers
2013-09-04 20:53 ` Cong Hou
2013-09-04 20:59 ` Joseph S. Myers
2013-09-04 21:21 ` Xinliang David Li
2013-09-04 21:48 ` Cong Hou
2013-09-04 22:26 ` Joseph S. Myers
2013-09-06 22:24 ` Cong Hou
2013-09-10 2:20 ` Xinliang David Li
2013-09-10 4:02 ` Cong Hou
2013-09-20 17:05 ` Cong Hou
2013-10-03 23:19 ` Cong Hou
2013-10-04 0:06 ` Joseph S. Myers
2013-10-07 17:15 ` Cong Hou
2013-10-17 18:30 ` Cong Hou
2013-10-23 21:13 ` Joseph S. Myers
2013-10-24 19:10 ` Cong Hou
2013-09-04 21:18 ` Xinliang David Li
2013-09-03 22:53 ` Bernhard Reutner-Fischer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).