Patchwork PATCH: Enable TARGET_HAS_SINCOS if x87 FPU fsincos is available

login
register
mail settings
Submitter H.J. Lu
Date Aug. 30, 2010, 3:53 p.m.
Message ID <AANLkTinuzk6qnVsOvGYxu06j2PfYdMh0_mACaTGKSk2B@mail.gmail.com>
Download mbox | patch
Permalink /patch/63063/
State New
Headers show

Comments

H.J. Lu - Aug. 30, 2010, 3:53 p.m.
On Mon, Aug 30, 2010 at 7:57 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, Aug 30, 2010 at 4:40 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>> Hi,
>>
>> On x86, sincos is always available if x87 FPU fsincos is available.
>> This patch enables TARGET_HAS_SINCOS for -ffast-math and x87 FPU.  Also
>> x86 Bionic C library doesn't provide sincos and we shouldn't enable
>> TARGET_HAS_SINCOS with OPTION_BIONIC on x86.  OK for trunk?
>
> Hm.  Shouldn't it be conditional on USE_FANCY_MATH_387
> && !NO_FANCY_MATH_387 as well?

You are right.  Here is the updated patch.

> Also a flag dependency breaks with the target/option attributes,

The current target/option attributes have many problems.  It
shouldn't prevent us from generating better codes.

> so I'm not sure it is a good idea.  Does anyone care for FP
> execution performance on 32bit anyway?
>

Intel cares FP execution performance on 32bit

Thanks.
t66667@gmail.com - Sept. 5, 2010, 4:01 a.m.
Hello:
On 31/08/2010 1:53 AM, H.J. Lu wrote:
> On Mon, Aug 30, 2010 at 7:57 AM, Richard Guenther
> <richard.guenther@gmail.com>  wrote:
>> On Mon, Aug 30, 2010 at 4:40 PM, H.J. Lu<hongjiu.lu@intel.com>  wrote:
>>> Hi,
>>>
>>> On x86, sincos is always available if x87 FPU fsincos is available.
>>> This patch enables TARGET_HAS_SINCOS for -ffast-math and x87 FPU.  Also
>>> x86 Bionic C library doesn't provide sincos and we shouldn't enable
>>> TARGET_HAS_SINCOS with OPTION_BIONIC on x86.  OK for trunk?
>>
>> Hm.  Shouldn't it be conditional on USE_FANCY_MATH_387
>> &&  !NO_FANCY_MATH_387 as well?
>
> You are right.  Here is the updated patch.
>
For your info, I used revision 163859, is this patch applied?
I get link error for the target arm-linux-androideabi: (sincos isn't 
used in the source)
imdct.c:(.text+0x27ac): undefined reference to `sincos'
imdct.c:(.text+0x281c): undefined reference to `sincos'
imdct.c:(.text+0x2894): undefined reference to `sincos'
imdct.c:(.text+0x2910): undefined reference to `sincos'
imdct.c:(.text+0x2980): undefined reference to `sincos'
Does this mean that gcc is deliberately looking for sincos?
>> Also a flag dependency breaks with the target/option attributes,
>
> The current target/option attributes have many problems.  It
> shouldn't prevent us from generating better codes.
>
>> so I'm not sure it is a good idea.  Does anyone care for FP
>> execution performance on 32bit anyway?
>>
>
> Intel cares FP execution performance on 32bit
>
> Thanks.
>
>
H.J. Lu - Sept. 5, 2010, 4:21 a.m.
On Sat, Sep 4, 2010 at 9:01 PM, t66667@gmail.com <t66667@gmail.com> wrote:
> Hello:
> On 31/08/2010 1:53 AM, H.J. Lu wrote:
>>
>> On Mon, Aug 30, 2010 at 7:57 AM, Richard Guenther
>> <richard.guenther@gmail.com>  wrote:
>>>
>>> On Mon, Aug 30, 2010 at 4:40 PM, H.J. Lu<hongjiu.lu@intel.com>  wrote:
>>>>
>>>> Hi,
>>>>
>>>> On x86, sincos is always available if x87 FPU fsincos is available.
>>>> This patch enables TARGET_HAS_SINCOS for -ffast-math and x87 FPU.  Also
>>>> x86 Bionic C library doesn't provide sincos and we shouldn't enable
>>>> TARGET_HAS_SINCOS with OPTION_BIONIC on x86.  OK for trunk?
>>>
>>> Hm.  Shouldn't it be conditional on USE_FANCY_MATH_387
>>> &&  !NO_FANCY_MATH_387 as well?
>>
>> You are right.  Here is the updated patch.
>>
> For your info, I used revision 163859, is this patch applied?

My patch only fixes x86, not ARM.

> I get link error for the target arm-linux-androideabi: (sincos isn't used in
> the source)
> imdct.c:(.text+0x27ac): undefined reference to `sincos'
> imdct.c:(.text+0x281c): undefined reference to `sincos'
> imdct.c:(.text+0x2894): undefined reference to `sincos'
> imdct.c:(.text+0x2910): undefined reference to `sincos'
> imdct.c:(.text+0x2980): undefined reference to `sincos'
> Does this mean that gcc is deliberately looking for sincos?

Gcc uses sincos if TARGET_HAS_SINCOS  is true.

Patch

diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
index 5bae99d..16a17c4 100644
--- a/gcc/config/i386/i386.h
+++ b/gcc/config/i386/i386.h
@@ -2492,6 +2492,13 @@  struct GTY(()) machine_function {
 #undef TARG_COND_NOT_TAKEN_BRANCH_COST
 #define TARG_COND_NOT_TAKEN_BRANCH_COST ix86_cost->cond_not_taken_branch_cost
 
+/* Use x87 FPU fsincos if it is available.  */
+#undef TARGET_HAS_SINCOS
+#define TARGET_HAS_SINCOS				\
+  (TARGET_USE_FANCY_MATH_387				\
+   && flag_unsafe_math_optimizations	 		\
+   && (!TARGET_SSE_MATH || TARGET_MIX_SSE_I387))
+
 /*
 Local variables:
 version-control: t
diff --git a/gcc/config/i386/linux.h b/gcc/config/i386/linux.h
index 2a31880..42db416 100644
--- a/gcc/config/i386/linux.h
+++ b/gcc/config/i386/linux.h
@@ -35,6 +35,16 @@  along with GCC; see the file COPYING3.  If not see
 #undef TARGET_TLS_DIRECT_SEG_REFS_DEFAULT
 #define TARGET_TLS_DIRECT_SEG_REFS_DEFAULT MASK_TLS_DIRECT_SEG_REFS
 
+/* Whether we have sincos that follows the GNU extension.  There is no
+   sincos in Bionic C library.  We can only use x87 FPU fsincos if it
+   is available.  */
+#undef TARGET_HAS_SINCOS
+#define TARGET_HAS_SINCOS				\
+  (OPTION_GLIBC						\
+   || (TARGET_USE_FANCY_MATH_387			\
+       && flag_unsafe_math_optimizations		\
+       && (!TARGET_SSE_MATH || TARGET_MIX_SSE_I387)))
+
 #undef ASM_COMMENT_START
 #define ASM_COMMENT_START "#"
 
diff --git a/gcc/config/i386/linux64.h b/gcc/config/i386/linux64.h
index 867de59..7bc12b7 100644
--- a/gcc/config/i386/linux64.h
+++ b/gcc/config/i386/linux64.h
@@ -50,6 +50,16 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #undef TARGET_TLS_DIRECT_SEG_REFS_DEFAULT
 #define TARGET_TLS_DIRECT_SEG_REFS_DEFAULT MASK_TLS_DIRECT_SEG_REFS
 
+/* Whether we have sincos that follows the GNU extension.  There is no
+   sincos in Bionic C library.  We can only use x87 FPU fsincos if it
+   is available.  */
+#undef TARGET_HAS_SINCOS
+#define TARGET_HAS_SINCOS				\
+  (OPTION_GLIBC						\
+   || (TARGET_USE_FANCY_MATH_387			\
+       && flag_unsafe_math_optimizations		\
+       && (!TARGET_SSE_MATH || TARGET_MIX_SSE_I387)))
+
 /* Provide a LINK_SPEC.  Here we provide support for the special GCC
    options -static and -shared, which allow us to link things in one
    of these three modes by applying the appropriate combinations of
diff --git a/gcc/config/linux.h b/gcc/config/linux.h
index e283a9a..576a2ac 100644
--- a/gcc/config/linux.h
+++ b/gcc/config/linux.h
@@ -159,7 +159,9 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
    is present in the runtime library.  */
 #define TARGET_C99_FUNCTIONS (OPTION_GLIBC)
 
+#ifndef TARGET_HAS_SINCOS
 /* Whether we have sincos that follows the GNU extension.  */
 #define TARGET_HAS_SINCOS (OPTION_GLIBC || OPTION_BIONIC)
+#endif
 
 #define TARGET_POSIX_IO