| Message ID | CAA42iKyaBrK0=D+ci_LrWDWapQj44bbLgx20XvyKEP-0LEaLnA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | libgcc libiberty: optimize and modernize standard string and memory functions | expand |
On 6/3/2021 12:51 PM, Seija K. via Gcc-patches wrote: > This patch optimizes and simplifies many of the standard string functions. > > Since C99, some of the standard string functions have been changed to use > the restrict modifier. > > diff --git a/libgcc/memcmp.c b/libgcc/memcmp.c > index 2348afe1d27f7..74195cf6baf13 100644 > --- a/libgcc/memcmp.c > +++ b/libgcc/memcmp.c > @@ -7,10 +7,11 @@ memcmp (const void *str1, const void *str2, size_t count) > const unsigned char *s1 = str1; > const unsigned char *s2 = str2; > > - while (count-- > 0) > + while (count--) > { > - if (*s1++ != *s2++) > - return s1[-1] < s2[-1] ? -1 : 1; > + if (*s1 != *s2) > + return *s1 < *s2 ? -1 : 1; > + s1++, s2++; > } > return 0; > } I don't see the point behind this change and the similar ones in other files. I'd like to see some justification for the change beyond just "this looks cleaner to me". > diff --git a/libgcc/memcpy.c b/libgcc/memcpy.c > index 58b1e405627aa..616df78fd2969 100644 > --- a/libgcc/memcpy.c > +++ b/libgcc/memcpy.c > @@ -2,7 +2,7 @@ > #include <stddef.h> > > void * > -memcpy (void *dest, const void *src, size_t len) > +memcpy (void * restrict dest, const void * restrict src, size_t len) I would expect prototype fixes like this within libgcc to be reasonably safe. > diff --git a/libiberty/memcpy.c b/libiberty/memcpy.c > index 7f67d0bd1f26c..d388ae7f3506b 100644 > --- a/libiberty/memcpy.c > +++ b/libiberty/memcpy.c > @@ -19,7 +19,7 @@ Copies @var{length} bytes from memory region > @var{in} to region > void bcopy (const void*, void*, size_t); > > PTR > -memcpy (PTR out, const PTR in, size_t length) > +memcpy (PTR restrict out, const PTR restrict in, size_t length) It's not entirely clear that using "restrict" is safe because libiberty is used by so many other projects which may be building with old compilers, non-gcc compilers, etc. Generally the way to handle this is to use an autoconf check to confirm that the tools can handle the feature you want to use and ensure there's a fallback when they can't. Jeff
On Wed, Jun 9, 2021 at 12:10 PM Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > On 6/3/2021 12:51 PM, Seija K. via Gcc-patches wrote: > > This patch optimizes and simplifies many of the standard string functions. > > > > Since C99, some of the standard string functions have been changed to use > > the restrict modifier. > > > > diff --git a/libgcc/memcmp.c b/libgcc/memcmp.c > > index 2348afe1d27f7..74195cf6baf13 100644 > > --- a/libgcc/memcmp.c > > +++ b/libgcc/memcmp.c > > @@ -7,10 +7,11 @@ memcmp (const void *str1, const void *str2, size_t count) > > const unsigned char *s1 = str1; > > const unsigned char *s2 = str2; > > > > - while (count-- > 0) > > + while (count--) > > { > > - if (*s1++ != *s2++) > > - return s1[-1] < s2[-1] ? -1 : 1; > > + if (*s1 != *s2) > > + return *s1 < *s2 ? -1 : 1; > > + s1++, s2++; > > } > > return 0; > > } > I don't see the point behind this change and the similar ones in other > files. I'd like to see some justification for the change beyond just > "this looks cleaner to me". > > > > > > > diff --git a/libgcc/memcpy.c b/libgcc/memcpy.c > > index 58b1e405627aa..616df78fd2969 100644 > > --- a/libgcc/memcpy.c > > +++ b/libgcc/memcpy.c > > @@ -2,7 +2,7 @@ > > #include <stddef.h> > > > > void * > > -memcpy (void *dest, const void *src, size_t len) > > +memcpy (void * restrict dest, const void * restrict src, size_t len) > I would expect prototype fixes like this within libgcc to be reasonably > safe. > > > diff --git a/libiberty/memcpy.c b/libiberty/memcpy.c > > index 7f67d0bd1f26c..d388ae7f3506b 100644 > > --- a/libiberty/memcpy.c > > +++ b/libiberty/memcpy.c > > @@ -19,7 +19,7 @@ Copies @var{length} bytes from memory region > > @var{in} to region > > void bcopy (const void*, void*, size_t); > > > > PTR > > -memcpy (PTR out, const PTR in, size_t length) > > +memcpy (PTR restrict out, const PTR restrict in, size_t length) > It's not entirely clear that using "restrict" is safe because libiberty > is used by so many other projects which may be building with old > compilers, non-gcc compilers, etc. > > Generally the way to handle this is to use an autoconf check to confirm > that the tools can handle the feature you want to use and ensure there's > a fallback when they can't. > The macro that autoconf provides for this purpose is AC_C_RESTRICT; it's documented here: https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.70/html_node/C-Compiler.html ...and defined here: http://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=lib/autoconf/c.m4;h=47434c89845e1a3f6b579cdac81aae5f27809daa;hb=HEAD#l2030 > Jeff
diff --git a/libgcc/memcmp.c b/libgcc/memcmp.c index 2348afe1d27f7..74195cf6baf13 100644 --- a/libgcc/memcmp.c +++ b/libgcc/memcmp.c @@ -7,10 +7,11 @@ memcmp (const void *str1, const void *str2, size_t count) const unsigned char *s1 = str1; const unsigned char *s2 = str2; - while (count-- > 0) + while (count--) { - if (*s1++ != *s2++) - return s1[-1] < s2[-1] ? -1 : 1; + if (*s1 != *s2) + return *s1 < *s2 ? -1 : 1; + s1++, s2++; } return 0; } diff --git a/libgcc/memcpy.c b/libgcc/memcpy.c index 58b1e405627aa..616df78fd2969 100644 --- a/libgcc/memcpy.c +++ b/libgcc/memcpy.c @@ -2,7 +2,7 @@ #include <stddef.h> void * -memcpy (void *dest, const void *src, size_t len) +memcpy (void * restrict dest, const void * restrict src, size_t len) { char *d = dest; const char *s = src; diff --git a/libgcc/memset.c b/libgcc/memset.c index 3e7025ee39443..b3b27cd63e12d 100644 --- a/libgcc/memset.c +++ b/libgcc/memset.c @@ -5,7 +5,7 @@ void * memset (void *dest, int val, size_t len) { unsigned char *ptr = dest; - while (len-- > 0) - *ptr++ = val; + while (len--) + *ptr++ = (unsigned char)val; return dest; } diff --git a/libiberty/memchr.c b/libiberty/memchr.c index 7448ab9e71c32..6f03e9c281108 100644 --- a/libiberty/memchr.c +++ b/libiberty/memchr.c @@ -23,7 +23,7 @@ memchr (register const PTR src_void, int c, size_t length) { const unsigned char *src = (const unsigned char *)src_void; - while (length-- > 0) + while (length--) { if (*src == c) return (PTR)src; diff --git a/libiberty/memcmp.c b/libiberty/memcmp.c index 37db60f38267a..f41b35a758cc4 100644 --- a/libiberty/memcmp.c +++ b/libiberty/memcmp.c @@ -27,8 +27,9 @@ memcmp (const PTR str1, const PTR str2, size_t count) while (count-- > 0) { - if (*s1++ != *s2++) - return s1[-1] < s2[-1] ? -1 : 1; + if (*s1 != *s2) + return *s1 < *s2 ? -1 : 1; + s1++, s2++; } return 0; } diff --git a/libiberty/memcpy.c b/libiberty/memcpy.c index 7f67d0bd1f26c..d388ae7f3506b 100644 --- a/libiberty/memcpy.c +++ b/libiberty/memcpy.c @@ -19,7 +19,7 @@ Copies @var{length} bytes from memory region @var{in} to region void bcopy (const void*, void*, size_t); PTR -memcpy (PTR out, const PTR in, size_t length) +memcpy (PTR restrict out, const PTR restrict in, size_t length) { bcopy(in, out, length); return out; diff --git a/libiberty/mempcpy.c b/libiberty/mempcpy.c index f4c624d4a3227..ac56eeaee0d5e 100644 --- a/libiberty/mempcpy.c +++ b/libiberty/mempcpy.c @@ -33,10 +33,10 @@ Copies @var{length} bytes from memory region @var{in} to region #include <ansidecl.h> #include <stddef.h> -extern PTR memcpy (PTR, const PTR, size_t); +extern PTR memcpy (PTR restrict, const PTR restrict, size_t); PTR -mempcpy (PTR dst, const PTR src, size_t len) +mempcpy (PTR restrict dst, const PTR restrict src, size_t len) { return (char *) memcpy (dst, src, len) + len;