Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h

[Sebastian Schuberth <sschuberth@gmail.com>]

On 18.08.2009 13:07, Junio C Hamano wrote:

> Your proposed commit log message makes it sound as if this change is
> limited to Windows, but it is not protected with "#ifdef WIN32"; the
> change should be applicable to non-windows but the message is misleading.

True, the change should apply for x86, no matter what the OS. I wanted to express that *on Windows* I know ntohl()/htonl() are function that do shifts internally. Maybe on some other OS  these have better implementations (for x86).

> It seems that your patch is linewrapped, so please be careful _if_ it
> needs to be modified and resent (if this version gets trivially acked I
> can fix it up when applying and in such a case there is no need to
> resend).

Here's an updated version of the patch that both improves the commit message and fixes the line wrap.

 From b4f40f73e8bf410c975b3f29d10ca779343e3095 Mon Sep 17 00:00:00 2001
From: Sebastian Schuberth <sschuberth@gmail.com>
Date: Tue, 18 Aug 2009 12:33:35 +0200
Subject: [PATCH] block-sha1: On x86, use bswap built-in in favor of ntohl()/htonl()

On x86 and compatible, use the native bswap instruction in favor of ntohl()/
htonl(). In the best case, this gets rid of function calls to crappy
implementations, in the worst case, it should be no slower than sane (inlined)
implementations. The current code depends on GCC already, so relying on
__builtin_bswap32() to be available should be safe.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
  block-sha1/sha1.c |   15 ++++++++++-----
  1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
index f2830c0..07f2937 100644
--- a/block-sha1/sha1.c
+++ b/block-sha1/sha1.c
@@ -66,15 +66,20 @@
  
  /*
   * Performance might be improved if the CPU architecture is OK with
- * unaligned 32-bit loads and a fast ntohl() is available.
+ * unaligned 32-bit loads and a fast ntohl() is available. On Intel,
+ * use the bswap built-in to get rid of the function call overhead.
   * Otherwise fall back to byte loads and shifts which is portable,
   * and is faster on architectures with memory alignment issues.
   */
  
-#if defined(__i386__) || defined(__x86_64__) || \
-    defined(__ppc__) || defined(__ppc64__) || \
-    defined(__powerpc__) || defined(__powerpc64__) || \
-    defined(__s390__) || defined(__s390x__)
+#if defined(__i386__) || defined(__x86_64__)
+
+#define get_be32(p)	__builtin_bswap32(*(unsigned int *)(p))
+#define put_be32(p, v)	do { *(unsigned int *)(p) = __builtin_bswap32(v); } while (0)
+
+#elif defined(__ppc__) || defined(__ppc64__) || \
+      defined(__powerpc__) || defined(__powerpc64__) || \
+      defined(__s390__) || defined(__s390x__)
  
  #define get_be32(p)	ntohl(*(unsigned int *)(p))
  #define put_be32(p, v)	do { *(unsigned int *)(p) = htonl(v); } while (0)
-- 
1.6.4.169.g64d5.dirty