This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fixes tree-loop-distribute-patterns issues
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- Cc: Roland McGrath <roland at hack dot frob dot com>, "GNU C. Library" <libc-alpha at sourceware dot org>, Siddhesh Poyarekar <siddhesh at redhat dot com>
- Date: Thu, 20 Jun 2013 13:50:04 -0400
- Subject: Re: [PATCH] Fixes tree-loop-distribute-patterns issues
- References: <51C0AFB7 dot 1060009 at linux dot vnet dot ibm dot com> <20130618205608 dot 9CCE22C0AC at topped-with-meat dot com> <51C1BFE9 dot 4070805 at linux dot vnet dot ibm dot com> <51C1CEFC dot 9000100 at redhat dot com> <51C1FE4C dot 3020400 at linux dot vnet dot ibm dot com> <20130619221130 dot 7B91A2C10E at topped-with-meat dot com> <51C31177 dot 90303 at linux dot vnet dot ibm dot com>
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.