Re: memchr2 speed, gcc

[Eric Blake]

Bruno Haible <bruno <at> clisp.org> writes:

> > +   New module 'memchr2'.
> 
> Wondering why you used 'uintmax_t' as basic word type, rather than the
> 'unsigned long' that memchr.c uses, I benchmarked this and a few other
> variations of the memchr2.c implementation.
> 
> Summary of results:
>   - With gcc 3.2.2 and 4.2.2, the word type 'unsigned long' is more efficient.
>   - With gcc 4.3-20080215, it is the opposite. But this version of gcc also
>     exhibits mysterious performance characteristics.

I've got an even more efficient implementation, inspired by 
http://www.cl.cam.ac.uk/~am21/progtricks.html, and here are my measurements of 
100000 iterations of your test program using gcc 3.4.4 at -O2 on cygwin (32-bit 
machine):

original     original     modified     modified
uintmax_t    uint32_t     uintmax_t    uint32_t
1.250        0.828        1.375        0.765 

As you observed in your measurements, the 32-bit operations were more efficient 
with this older gcc.  But what is more interesting is that the modified version 
was roughly 10% worse than the original with 64-bit math, while it was roughly 
8% better with 32-bit math.  I'm not sure what is causing that effect (maybe 
it's due to more register pressure for the number of live values during the 
computation; maybe it's due to the fact that the original version can be proven 
to not trigger a carry bit to cross the 32-bit boundary, but the subtraction in 
the modified version can cause a carry, and carries slow down 64-bit math when 
done in 32-bit chunks).  But with the right data type, the modified version is 
inherently faster, since it requires fewer operations and never has false hits.

Therefore, I'm checking this in, although it would be interesting to see your 
timings by swapping the typedef to try 64-bit math with newer gcc again.  For 
that matter, is it worth putting a preprocessor conditional to change the 
typedef according to whether the compiler appears to be a new enough version of 
gcc to intelligently optimize the 64-bit math?

Also, is anyone interested in making gnulib's memchr and strchrnul more 
efficient by copying the optimizations learned in memchr2?  Also, should we 
provide a replacement for glibc's rawmemchr (surprisingly useful:
rawmemchr(s,0) is more efficient than a naive strchr(s,0) or s+strlen(s))?

>From 092621651ba401797a2e671eb24207c49e5e9b64 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Wed, 23 Apr 2008 15:03:40 -0600
Subject: [PATCH] Improve memchr2 performance.

* lib/memchr2.c (memchr2): Further optimize parallel detection of
NUL bytes.
* modules/memchr2 (Depends-on): Use intprops.h.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog       |    7 +++
 lib/memchr2.c   |  140 ++++++++++++++----------------------------------------
 modules/memchr2 |    1 +
 3 files changed, 45 insertions(+), 103 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4b87fda..769f0f3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2008-04-23  Eric Blake  <address@hidden>
+
+       Improve memchr2 performance.
+       * lib/memchr2.c (memchr2): Further optimize parallel detection of
+       NUL bytes.
+       * modules/memchr2 (Depends-on): Use intprops.h.
+
 2008-04-23  Simon Josefsson  <address@hidden>
 
        * lib/sys_socket.in.h (setsockopt): Be more type safe by declaring
diff --git a/lib/memchr2.c b/lib/memchr2.c
index 3853343..81de613 100644
--- a/lib/memchr2.c
+++ b/lib/memchr2.c
@@ -29,19 +29,28 @@ along with this program.  If not, see 
<http://www.gnu.org/licenses/>.  */
 #include <stdint.h>
 #include <string.h>
 
+#include "intprops.h"
+
 /* Return the first address of either C1 or C2 (treated as unsigned
    char) that occurs within N bytes of the memory region S.  If
    neither byte appears, return NULL.  */
 void *
 memchr2 (void const *s, int c1_in, int c2_in, size_t n)
 {
+  /* On 32-bit hardware, choosing longword to be a 32-bit unsigned
+     long instead of a 64-bit uintmax_t tends to give better
+     performance.  On 64-bit hardware, unsigned long is generally 64
+     bits already.  Change this typedef to experiment with
+     performance.  */
+  typedef unsigned long longword;
+
   const unsigned char *char_ptr;
-  const uintmax_t *longword_ptr;
-  uintmax_t longword1;
-  uintmax_t longword2;
-  uintmax_t magic_bits;
-  uintmax_t charmask1;
-  uintmax_t charmask2;
+  const longword *longword_ptr;
+  longword longword1;
+  longword longword2;
+  longword magic_bits;
+  longword charmask1;
+  longword charmask2;
   unsigned char c1;
   unsigned char c2;
   int i;
@@ -63,31 +72,17 @@ memchr2 (void const *s, int c1_in, int c2_in, size_t n)
   /* All these elucidatory comments refer to 4-byte longwords,
      but the theory applies equally well to any size longwords.  */
 
-  longword_ptr = (const uintmax_t *) char_ptr;
-
-  /* Bits 31, 24, 16, and 8 of this number are zero.  Call these bits
-     the "holes."  Note that there is a hole just to the left of
-     each byte, with an extra at the end:
-
-     bits:  01111110 11111110 11111110 11111111
-     bytes: AAAAAAAA BBBBBBBB CCCCCCCC DDDDDDDD
-
-     The 1-bits make sure that carries propagate to the next 0-bit.
-     The 0-bits provide holes for carries to fall into.  */
-
-  /* Set MAGIC_BITS to be this pattern of 1 and 0 bits.
-     Set CHARMASK to be a longword, each of whose bytes is C.  */
-
-  magic_bits = 0xfefefefe;
+  longword_ptr = (const longword *) char_ptr;
+  magic_bits = 0x01010101;
   charmask1 = c1 | (c1 << 8);
   charmask2 = c2 | (c2 << 8);
   charmask1 |= charmask1 << 16;
   charmask2 |= charmask2 << 16;
-  if (0xffffffffU < UINTMAX_MAX)
+  if (0xffffffffU < TYPE_MAXIMUM (longword))
     {
-      magic_bits |= magic_bits << 32;
-      charmask1 |= charmask1 << 32;
-      charmask2 |= charmask2 << 32;
+      magic_bits |= magic_bits << 31 << 1;
+      charmask1 |= charmask1 << 31 << 1;
+      charmask2 |= charmask2 << 31 << 1;
       if (8 < sizeof longword1)
        for (i = 64; i < sizeof longword1 * 8; i *= 2)
          {
@@ -96,89 +91,28 @@ memchr2 (void const *s, int c1_in, int c2_in, size_t n)
            charmask2 |= charmask2 << i;
          }
     }
-  magic_bits = (UINTMAX_MAX >> 1) & (magic_bits | 1);
 
   /* Instead of the traditional loop which tests each character,
      we will test a longword at a time.  The tricky part is testing
-     if *any of the four* bytes in the longword in question are zero.  */
+     if *any of the four* bytes in the longword in question are zero.
+
+     We first use an xor to convert target bytes into a NUL byte,
+     since the test for a zero byte is more efficient.  For all byte
+     values except 0x00 and 0x80, subtracting 1 from the byte will
+     leave the most significant bit unchanged.  So detecting 0 is
+     simply a matter of subtracting from all bytes in parallel, and
+     checking for a most significant bit that changed to 1.  */
+
   while (n >= sizeof longword1)
     {
-      /* We tentatively exit the loop if adding MAGIC_BITS to
-        LONGWORD fails to change any of the hole bits of LONGWORD.
-
-        1) Is this safe?  Will it catch all the zero bytes?
-        Suppose there is a byte with all zeros.  Any carry bits
-        propagating from its left will fall into the hole at its
-        least significant bit and stop.  Since there will be no
-        carry from its most significant bit, the LSB of the
-        byte to the left will be unchanged, and the zero will be
-        detected.
-
-        2) Is this worthwhile?  Will it ignore everything except
-        zero bytes?  Suppose every byte of LONGWORD has a bit set
-        somewhere.  There will be a carry into bit 8.  If bit 8
-        is set, this will carry into bit 16.  If bit 8 is clear,
-        one of bits 9-15 must be set, so there will be a carry
-        into bit 16.  Similarly, there will be a carry into bit
-        24.  If one of bits 24-30 is set, there will be a carry
-        into bit 31, so all of the hole bits will be changed.
-
-        The one misfire occurs when bits 24-30 are clear and bit
-        31 is set; in this case, the hole at bit 31 is not
-        changed.  If we had access to the processor carry flag,
-        we could close this loophole by putting the fourth hole
-        at bit 32!
-
-        So it ignores everything except 128's, when they're aligned
-        properly.
-
-        3) But wait!  Aren't we looking for C, not zero?
-        Good point.  So what we do is XOR LONGWORD with a longword,
-        each of whose bytes is C.  This turns each byte that is C
-        into a zero.  */
-
       longword1 = *longword_ptr ^ charmask1;
-      longword2 = *longword_ptr++ ^ charmask2;
-
-      /* Add MAGIC_BITS to LONGWORD.  */
-      if ((((longword1 + magic_bits)
-
-           /* Set those bits that were unchanged by the addition.  */
-           ^ ~longword1)
-
-          /* Look at only the hole bits.  If any of the hole bits
-             are unchanged, most likely one of the bytes was a
-             zero.  */
-          & ~magic_bits) != 0
-         || (((longword2 + magic_bits) ^ ~longword2) & ~magic_bits) != 0)
-       {
-         /* Which of the bytes was C?  If none of them were, it was
-            a misfire; continue the search.  */
-
-         const unsigned char *cp = (const unsigned char *) (longword_ptr - 1);
-
-         if (cp[0] == c1 || cp[0] == c2)
-           return (void *) cp;
-         if (cp[1] == c1 || cp[1] == c2)
-           return (void *) &cp[1];
-         if (cp[2] == c1 || cp[2] == c2)
-           return (void *) &cp[2];
-         if (cp[3] == c1 || cp[3] == c2)
-           return (void *) &cp[3];
-         if (4 < sizeof longword1 && (cp[4] == c1 || cp[4] == c2))
-           return (void *) &cp[4];
-         if (5 < sizeof longword1 && (cp[5] == c1 || cp[5] == c2))
-           return (void *) &cp[5];
-         if (6 < sizeof longword1 && (cp[6] == c1 || cp[6] == c2))
-           return (void *) &cp[6];
-         if (7 < sizeof longword1 && (cp[7] == c1 || cp[7] == c2))
-           return (void *) &cp[7];
-         if (8 < sizeof longword1)
-           for (i = 8; i < sizeof longword1; i++)
-             if (cp[i] == c1 || cp[i] == c2)
-               return (void *) &cp[i];
-       }
+      longword2 = *longword_ptr ^ charmask2;
 
+      if (((((longword1 - magic_bits) & ~longword1)
+           | ((longword2 - magic_bits) & ~longword2))
+          & (magic_bits << 7)) != 0)
+       break;
+      longword_ptr++;
       n -= sizeof longword1;
     }
 
@@ -191,5 +125,5 @@ memchr2 (void const *s, int c1_in, int c2_in, size_t n)
       ++char_ptr;
     }
 
-  return 0;
+  return NULL;
 }
diff --git a/modules/memchr2 b/modules/memchr2
index 7e83415..da5671d 100644
--- a/modules/memchr2
+++ b/modules/memchr2
@@ -6,6 +6,7 @@ lib/memchr2.h
 lib/memchr2.c
 
 Depends-on:
+intprops
 stdint
 memchr
 
-- 
1.5.5.1