Re: [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions

[Jeff King]

On Sun, Jul 16, 2017 at 06:27:04AM -0400, Jeff King wrote:

> On Sat, Jul 15, 2017 at 09:22:50PM +0200, René Scharfe wrote:
> 
> > Simplify the implementation and allow callers to use expressions with
> > side-effects by turning the macros get_be16, get_be32 and put_be32 into
> > inline functions.
> > 
> > Signed-off-by: Rene Scharfe 
> > ---
> > All these redundant casts started to bother me, so I tried to come up
> > with nice and clean inline functions.  Successfully?  You tell me.
> > They are longer, but less cluttered.  Would it punish -O0 builds?  Is
> > it all worth it?
> 
> I do think the end result is a lot more readable. On gcc 6 at least, the
> function seems[1] to end up inlined even with -O0.

For my footnote. I was just going to show the test file I compiled:

  #include "git-compat-util.h"
  uint32_t foo(const char *x)
  {
return get_be32(x);
  }

It's possible the optimizer may behave differently on a more complicated
input, but it does show that -O0 is still willing to inline.

-Peff

Re: [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions

[Jeff King]

On Sat, Jul 15, 2017 at 09:22:50PM +0200, René Scharfe wrote:

> Simplify the implementation and allow callers to use expressions with
> side-effects by turning the macros get_be16, get_be32 and put_be32 into
> inline functions.
> 
> Signed-off-by: Rene Scharfe 
> ---
> All these redundant casts started to bother me, so I tried to come up
> with nice and clean inline functions.  Successfully?  You tell me.
> They are longer, but less cluttered.  Would it punish -O0 builds?  Is
> it all worth it?

I do think the end result is a lot more readable. On gcc 6 at least, the
function seems[1] to end up inlined even with -O0.

Interestingly, at -O2 even with -DNO_UNALIGNED_LOADS, gcc converts the
result to a movl and a bswap. Which is the same thing our
unaligned-loads path is trying for. I wonder if we could/should just
drop it (that _would_ punish -O0 on x86, though).

-Peff

[PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions

[René Scharfe]

Simplify the implementation and allow callers to use expressions with
side-effects by turning the macros get_be16, get_be32 and put_be32 into
inline functions.

Signed-off-by: Rene Scharfe 
---
All these redundant casts started to bother me, so I tried to come up
with nice and clean inline functions.  Successfully?  You tell me.
They are longer, but less cluttered.  Would it punish -O0 builds?  Is
it all worth it?

 compat/bswap.h | 38 --
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/compat/bswap.h b/compat/bswap.h
index 4582c1107a..7d063e9e40 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -162,19 +162,29 @@ static inline uint64_t git_bswap64(uint64_t x)
 
 #else
 
-#define get_be16(p)( \
-   (*((unsigned char *)(p) + 0) << 8) | \
-   (*((unsigned char *)(p) + 1) << 0) )
-#define get_be32(p)( \
-   ((uint32_t)*((unsigned char *)(p) + 0) << 24) | \
-   ((uint32_t)*((unsigned char *)(p) + 1) << 16) | \
-   ((uint32_t)*((unsigned char *)(p) + 2) <<  8) | \
-   ((uint32_t)*((unsigned char *)(p) + 3) <<  0) )
-#define put_be32(p, v) do { \
-   unsigned int __v = (v); \
-   *((unsigned char *)(p) + 0) = __v >> 24; \
-   *((unsigned char *)(p) + 1) = __v >> 16; \
-   *((unsigned char *)(p) + 2) = __v >>  8; \
-   *((unsigned char *)(p) + 3) = __v >>  0; } while (0)
+static inline uint16_t get_be16(const void *ptr)
+{
+   const unsigned char *p = ptr;
+   return  (uint16_t)p[0] << 8 |
+   (uint16_t)p[1] << 0;
+}
+
+static inline uint32_t get_be32(const void *ptr)
+{
+   const unsigned char *p = ptr;
+   return  (uint32_t)p[0] << 24 |
+   (uint32_t)p[1] << 16 |
+   (uint32_t)p[2] <<  8 |
+   (uint32_t)p[3] <<  0;
+}
+
+static inline void put_be32(void *ptr, uint32_t value)
+{
+   unsigned char *p = ptr;
+   p[0] = value >> 24;
+   p[1] = value >> 16;
+   p[2] = value >>  8;
+   p[3] = value >>  0;
+}
 
 #endif
-- 
2.13.3