This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


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

Re: [PATCH] Fixes tree-loop-distribute-patterns issues


On 06/20/2013 10:28 AM, Adhemerval Zanella wrote:
> On 19-06-2013 19:11, Roland McGrath wrote:
>>>> I'm a little worried about the fragility of the test, but if it doesn't
>>>> optimize to memset, then it may not optimize the internal implementation
>>>> to memset, and therefore we need not avoid the benefit of the optimization.
>>>>
>>>> I think this is a good approach, but we will have to see how it plays out
>>>> and if it is robust enough.
>>> We might make it more robust (maybe trying different loops), do you think it 
>>> is worth to push more tests?
>> I had not contemplated this sort of configure check and I don't think it's
>> what we want at all.  It doesn't matter whether the compiler actually
>> transforms a particular case into a call.  All that matters is that the
>> compiler understands
>> __attribute__ ((optimize ("-fno-tree-loop-distribute-patterns"))).
>> If it does, then we'll use it every place we think it might matter.
>>
>>
> I have changed the patch so the configure now only checks for support of
> the compiler flag. I also changed the names as you suggested.

One problem, needs more comments :-)

OK for me with comment added.

> ---
> 
> 2013-06-20  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
> 
> 	* config.h.in (HAVE_CC_INHIBIT_LOOP_TO_LIBCALL): New define.
> 	* configure.in (libc_cv_cc_loop_to_function): Check if compiler
> 	accepts -fno-tree-loop-distribute-patterns.
> 	* include/libc-symbols.h (inhibit_loop_to_libcall): New macro.
> 	* string/memmove.c (MEMMOVE): Disable loop transformation to avoid
> 	recursive call.
> 	* string/memset.c (memset): Likewise.
> 	* string/test-memmove.c (simple_memmove): Disable loop transformation
> 	to library calls.
> 	* string/test-memset.c (simple_memset): Likewise.
> 	* benchtests/bench-memmove.c (simple_memmove): Likewise.
> 	* benchtests/bench-memset.c (simple_memset): Likewise.
> 	* configure: Regenerated.
> 
> --
> 
> diff --git a/benchtests/bench-memmove.c b/benchtests/bench-memmove.c
> index dccde5d..8925606 100644
> --- a/benchtests/bench-memmove.c
> +++ b/benchtests/bench-memmove.c
> @@ -46,6 +46,7 @@ IMPL (memmove, 1)
>  #endif
>  
>  char *
> +inhibit_loop_to_libcall
>  simple_memmove (char *dst, const char *src, size_t n)
>  {
>    char *ret = dst;
> diff --git a/benchtests/bench-memset.c b/benchtests/bench-memset.c
> index 92e34f0..ea29cf3 100644
> --- a/benchtests/bench-memset.c
> +++ b/benchtests/bench-memset.c
> @@ -63,6 +63,7 @@ builtin_memset (char *s, int c, size_t n)
>  #endif
>  
>  char *
> +inhibit_loop_to_libcall
>  simple_memset (char *s, int c, size_t n)
>  {
>    char *r = s, *end = s + n;
> diff --git a/config.h.in b/config.h.in
> index 8c2479e..b5c6f16 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -69,6 +69,9 @@
>  /* Define if the compiler supports __builtin_memset.  */
>  #undef	HAVE_BUILTIN_MEMSET
>  
> +/* Define if compiler accepts -ftree-loop-distribute-patterns.  */
> +#undef  HAVE_CC_INHIBIT_LOOP_TO_LIBCALL
> +
>  /* Define if the regparm attribute shall be used for local functions
>     (gcc on ix86 only).  */
>  #undef	USE_REGPARMS
> diff --git a/configure.in b/configure.in
> index 8b11081..479751b 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -1964,6 +1964,23 @@ if test -n "$submachine"; then
>  fi
>  AC_SUBST(libc_cv_cc_submachine)
>  
> +AC_CACHE_CHECK(for -fno-tree-loop-distribute-patterns, 
> +libc_cv_cc_loop_to_function, [dnl
> +cat > conftest.c <<EOF
> +void foo (void) {}
> +EOF
> +libc_cv_cc_loop_to_function=no
> +if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS -c
> +			    -fno-tree-loop-distribute-patterns conftest.c])
> +then
> +  libc_cv_cc_loop_to_function=yes
> +fi
> +rm -f conftest*])
> +if test $libc_cv_cc_loop_to_function = yes; then
> +  AC_DEFINE(HAVE_CC_INHIBIT_LOOP_TO_LIBCALL)
> +fi
> +AC_SUBST(libc_cv_cc_loop_to_function)
> +
>  dnl Check whether we have the gd library available.
>  AC_MSG_CHECKING(for libgd)
>  if test "$with_gd" != "no"; then
> diff --git a/include/libc-symbols.h b/include/libc-symbols.h
> index f043ce0..c7155b7 100644
> --- a/include/libc-symbols.h
> +++ b/include/libc-symbols.h
> @@ -782,4 +782,11 @@ for linking")
>  #define libc_ifunc_hidden_def(name) \
>    libc_ifunc_hidden_def1 (__GI_##name, name)
>  

This needs a detailed comment explaining why we are doing this.

The comment you had previously in the configure.in is sufficient IMO.

> +#ifdef HAVE_CC_INHIBIT_LOOP_TO_LIBCALL
> +# define inhibit_loop_to_libcall \
> +    __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns")))
> +#else
> +# define inhibit_loop_to_libcall
> +#endif
> +
>  #endif /* libc-symbols.h */
> diff --git a/string/memmove.c b/string/memmove.c
> index 9dcd2f1..ec61e28 100644
> --- a/string/memmove.c
> +++ b/string/memmove.c
> @@ -41,6 +41,7 @@
>  #endif
>  
>  rettype
> +inhibit_loop_to_libcall
>  MEMMOVE (a1, a2, len)
>       a1const void *a1;
>       a2const void *a2;
> diff --git a/string/memset.c b/string/memset.c
> index 868be53..74b5409 100644
> --- a/string/memset.c
> +++ b/string/memset.c
> @@ -21,6 +21,7 @@
>  #undef memset
>  
>  void *
> +inhibit_loop_to_libcall
>  memset (dstpp, c, len)
>       void *dstpp;
>       int c;
> diff --git a/string/test-memmove.c b/string/test-memmove.c
> index 4ec55b2..94576a2 100644
> --- a/string/test-memmove.c
> +++ b/string/test-memmove.c
> @@ -47,6 +47,7 @@ IMPL (memmove, 1)
>  #endif
>  
>  char *
> +inhibit_loop_to_libcall
>  simple_memmove (char *dst, const char *src, size_t n)
>  {
>    char *ret = dst;
> diff --git a/string/test-memset.c b/string/test-memset.c
> index 9981fce..72a390e 100644
> --- a/string/test-memset.c
> +++ b/string/test-memset.c
> @@ -64,6 +64,7 @@ builtin_memset (char *s, int c, size_t n)
>  #endif
>  
>  char *
> +inhibit_loop_to_libcall
>  simple_memset (char *s, int c, size_t n)
>  {
>    char *r = s, *end = s + n;
> 

Cheers,
Carlos.


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