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: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- To: "Carlos O'Donell" <carlos at redhat 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: Wed, 19 Jun 2013 15:54:04 -0300
- 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>
On 19-06-2013 12:32, Carlos O'Donell wrote:
> On 06/19/2013 10:27 AM, Adhemerval Zanella wrote:
>> On 18-06-2013 17:56, Roland McGrath wrote:
>>> What was discussed before was using a configure check. In general it's
>>> better to use empirical checks rather than version checks, even if in
>>> practice a version check is going to suffice. In this case, it does not
>>> seem difficult to write a configure check.
>>>
>>> Other macros of this sort we have are defined in lowercase
>>> (internal_function, etc.). Some of those have names indicative of their
>>> important semantics rather than echoing the syntax of the implementation.
>>> I think we should use such a name for this case too. I haven't thought of
>>> a particularly good name, but I don't think it should include "tree",
>>> "distribute", or "patterns".
>>>
>>> Attributes can appear in function definitions, right before the return
>>> type. There's no need for a separate forward declaration.
>>>
>>> The most obvious place we need these annotations is on the simple_*
>>> functions in string/test-*.c, so I would start by adding it to all those.
>>> That seems like the right initial set that goes in the change that
>>> introduces the macro and serves as implicit documentation of its use.
>>> Then we can take on each actual implementation that might be affected.
>>>
>>>
>>> Thanks,
>>> Roland
>>>
>> Thanks for the reviews, I change it to use a configure check instead and
>> changed the compiler attribute name a simpler one. I also added the attribute
>> on the string/test-*.c as you suggested. I put all the changes on one patch
>> since the string/memset.c and string/memmove.c modification are required so
>> loader can work correctly.
> Thanks, this is looking better and is exactly what I was thinking of.
>
> We check for the feature, set a feature macro, and avoid relying on the
> compiler version.
Thanks Carlos for the review, I modified the patch to incorporate the
changes you proposed.
> 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?
Below the proposed patch.
---
2013-06-19 Adhemerval Zanella <azanella@linux.vnet.ibm.com>
* config.h.in (HAVE_CC_PREDEF_DISTRIBUTE_PATTERNS): New define.
* configure.in (libc_cv_predef_loop_distribute_patttern): Check if
compiler plus optimization may generate memset/memmove library calls
for some loop constructions.
* include/libc-symbols.h (libc_disable_loop_to_calls): Define to
disable loop transformation to library calls.
* 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.
--
diff --git a/benchtests/bench-memmove.c b/benchtests/bench-memmove.c
index dccde5d..bfe085b 100644
--- a/benchtests/bench-memmove.c
+++ b/benchtests/bench-memmove.c
@@ -45,7 +45,7 @@ IMPL (simple_memmove, 0)
IMPL (memmove, 1)
#endif
-char *
+libc_disable_cc_loop_to_function char *
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..ac1d52d 100644
--- a/benchtests/bench-memset.c
+++ b/benchtests/bench-memset.c
@@ -62,7 +62,7 @@ builtin_memset (char *s, int c, size_t n)
}
#endif
-char *
+libc_disable_cc_loop_to_function char *
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..ad4d69e 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 implicitly defines -ftree-loop-distribute-patterns. */
+#undef HAVE_CC_LOOP_TO_FUNCTION
+
/* 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..38ac138 100644
--- a/configure.in
+++ b/configure.in
@@ -1964,6 +1964,32 @@ if test -n "$submachine"; then
fi
AC_SUBST(libc_cv_cc_submachine)
+dnl Check wheter the compiler may replace some loop constructions
+dnl by function calls to memset or memmove. This replacement is
+dnl problematic since it may lead to recursive calls in the
+dnl implementation of memset or memmove. We use this check to
+dnl detect the optimization and disable it if required.
+AC_CACHE_CHECK([whether $CC $CFLAGS implicitly replaces loops constructions \
+by function calls],
+ libc_cv_cc_loop_to_function, [
+ cat > conftest.c << EOF
+void foo (int *vec, int size) {
+ int i=0;
+ int *p = vec;
+ for (i=0; i<size; ++i, ++p)
+ *p = 0;
+}
+EOF
+libc_cv_cc_loop_to_function=no
+ if ${CC-cc} $CFLAGS -c -S conftest.c -o - | fgrep "memset" > /dev/null; 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_LOOP_TO_FUNCTION)
+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..85c8244 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)
+#ifdef HAVE_CC_LOOP_TO_FUNCTION
+# define libc_disable_cc_loop_to_function \
+ __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns")))
+#else
+# define libc_disable_cc_loop_to_function
+#endif
+
#endif /* libc-symbols.h */
diff --git a/string/memmove.c b/string/memmove.c
index 9dcd2f1..2b98c4a 100644
--- a/string/memmove.c
+++ b/string/memmove.c
@@ -40,7 +40,7 @@
#define MEMMOVE memmove
#endif
-rettype
+libc_disable_cc_loop_to_function rettype
MEMMOVE (a1, a2, len)
a1const void *a1;
a2const void *a2;
diff --git a/string/memset.c b/string/memset.c
index 868be53..9c62512 100644
--- a/string/memset.c
+++ b/string/memset.c
@@ -20,7 +20,7 @@
#undef memset
-void *
+libc_disable_cc_loop_to_function void *
memset (dstpp, c, len)
void *dstpp;
int c;
diff --git a/string/test-memmove.c b/string/test-memmove.c
index 4ec55b2..267756a 100644
--- a/string/test-memmove.c
+++ b/string/test-memmove.c
@@ -46,7 +46,7 @@ IMPL (simple_memmove, 0)
IMPL (memmove, 1)
#endif
-char *
+libc_disable_cc_loop_to_function char *
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..a6a1dcf 100644
--- a/string/test-memset.c
+++ b/string/test-memset.c
@@ -63,7 +63,7 @@ builtin_memset (char *s, int c, size_t n)
}
#endif
-char *
+libc_disable_cc_loop_to_function char *
simple_memset (char *s, int c, size_t n)
{
char *r = s, *end = s + n;
* benchtests/bench-memset.c (simple_memset): Likewise.
* configure: Regenerated.