svn commit: r232266 - in head/sys: amd64/include i386/include pc98/include x86/include

Fri Mar 2 20:29:17 UTC 2012

On Friday 02 March 2012 12:52:41 Bruce Evans wrote:
> On Fri, 2 Mar 2012, Bruce Evans wrote:
> 
> > On Thu, 1 Mar 2012, Tijl Coosemans wrote:
> >
> >> On Wednesday 29 February 2012 08:44:54 Bruce Evans wrote:
> >>> ...
> >>> So any macro version must use gcc features to be safe.  The following
> >>> seems to work:
> >>> 
> >>> #define	__bswap16(x)	__extension__ ({ __uint16_t __x = x;
> >>>  	(__uint16_t)(__x << 8 | __x >> 8); })
> >>> 
> >>> clang now produces "rolw $8,x" when x is volatile.  This seems to
> >>> violate volatile semantics.  gcc produces load-rolw-store then.  Both
> >>> produce "rolw $8,x" when x is not volatile.
> >> 
> >> I'll have a closer look at the patch tomorrow, but the Intel
> >> documentation for the bswap instruction recommends to use xchg for
> >> bswap16.
> >
> > That's what i386 used to do, using an asm (see for example the RELENG_4
> > version), but the asm was replaced by C code and the compiler apparently
> > knows better.  This might depend on -mtune.  I tested with the default,
> > ...
> 
> Here is another version.  It has now been tested a bit at runtime.
> I had to restore almost all of the complications, so the following
> reduces to mostly style changes including comments about surprising
> details.  It has 1 fix for clang (the comitted version doesn't
> work for clang).
> 
> % Index: endian.h
> % ===================================================================
> % RCS file: /home/ncvs/src/sys/x86/include/endian.h,v
> % retrieving revision 1.1
> % diff -u -2 -r1.1 endian.h
> % --- endian.h	28 Feb 2012 19:39:54 -0000	1.1
> % +++ endian.h	2 Mar 2012 10:43:53 -0000
> % @@ -37,8 +37,4 @@
> %  #include <sys/_types.h>
> % 
> % -#ifdef __cplusplus
> % -extern "C" {
> % -#endif
> % -
> %  /*
> %   * Define the order of 32-bit words in 64-bit words.
> % @@ -68,25 +64,17 @@
> %  #endif
> % 
> % -#if defined(__GNUCLIKE_ASM) && defined(__GNUCLIKE_BUILTIN_CONSTANT_P)
> % -
> % -#define	__bswap16_const(_x)	(__uint16_t)((_x) << 8 | (_x) >> 8)
> % +#define	__bswap16_gen(x)	(__uint16_t)((x) << 8 | (x) >> 8)
> % +#define	__bswap32_gen(x)		\
> % +	(((__uint32_t)__bswap16(x) << 16) | __bswap16((x) >> 16))
> % +#define	__bswap64_gen(x)		\
> % +	(((__uint64_t)__bswap32(x) << 32) | __bswap32((x) >> 32))
> % 
> % -#define	__bswap16(_x)			\
> % -	(__builtin_constant_p(_x) ?	\
> % -	    __bswap16_const((__uint16_t)(_x)) : __bswap16_var(_x))
> % -
> % -#define	__bswap32_const(_x)		\
> % -	(((__uint32_t)__bswap16(_x) << 16) | __bswap16((_x) >> 16))
> % -
> % -#define	__bswap32(_x)			\
> % -	(__builtin_constant_p(_x) ?	\
> % -	    __bswap32_const((__uint32_t)(_x)) : __bswap32_var(_x))
> % +#if defined(__GNUCLIKE_ASM) && defined(__GNUCLIKE_BUILTIN_CONSTANT_P)
> % 
> % -#define	__bswap64_const(_x)		\
> % -	(((__uint64_t)__bswap32(_x) << 32) | __bswap32((_x) >> 32))
> % +/* The following mess is to avoid multiple evaluation of x. */
> % 
> % -#define	__bswap64(_x)			\
> % -	(__builtin_constant_p(_x) ?	\
> % -	    __bswap64_const((__uint64_t)(_x)) : __bswap64_var(_x))
> % +#define	__bswap16(x)			\
> % +	(__builtin_constant_p(x) ?	\
> % +	    __bswap16_gen((__uint16_t)(x)) : __bswap16_var((__uint16_t)(x)))
> 
> For clang, x must be cast in the `var' clause too, since when x is
> something like 0x12345678 (having been passed here without conversion
> by __bswap32()), clang parses the `var' clause and warns about the
> implicit truncation of 0x12345678 to 0x5678 in it.  This is a bug
> in clang -- when warning about fine details, it is important not to
> do it for non-problems.
> 
> % 
> %  static __inline __uint16_t
> % @@ -94,7 +82,19 @@
> %  {
> % 
> % -	return (__bswap16_const(_x));
> % +	return (__bswap16_gen(_x));
> %  }
> % 
> % +/*
> % + * The following messes are old optimizations for gcc.  The messes have
> % + * been reduced significantly, but I don't know how to implement the
> % + * preferred way of defining defaults above and overriding them cleanly
> % + * here.  Even clang needs help for the 64-bit case, so to keep the
> % + * ifdefs sane we use this for the 32-bit case with clang too.
> % + */
> % +
> % +#define	__bswap32(x)			\
> % +	(__builtin_constant_p(x) ?	\
> % +	    __bswap32_gen((__uint32_t)(x)) : __bswap32_var(x))
> % +
> %  static __inline __uint32_t
> %  __bswap32_var(__uint32_t _x)
> % @@ -105,34 +105,37 @@
> %  }
> % 
> % +#define	__bswap64(x)			\
> % +	(__builtin_constant_p(x) ?	\
> % +	    __bswap64_gen((__uint64_t)(x)) : __bswap64_var(x))
> % +
> %  static __inline __uint64_t
> %  __bswap64_var(__uint64_t _x)
> %  {
> % -#ifdef	_LP64
> % +
> % +#ifdef _LP64
> %  	__asm ("bswap %0" : "+r" (_x));
> %  	return (_x);
> %  #else
> % -	return (__bswap64_const(_x));
> % +	/*
> % +	 * It is important for the optimizations that the following is not
> % +	 * really generic, but expands to 2 __bswap32_var()'s.
> % +	 */
> % +	return (__bswap64_gen(_x));
> %  #endif
> %  }
> % 
> % -#define	__htonl(x)	__bswap32(x)
> % -#define	__htons(x)	__bswap16(x)
> % -#define	__ntohl(x)	__bswap32(x)
> % -#define	__ntohs(x)	__bswap16(x)
> % +#else /* !(__GNUCLIKE_ASM && __GNUCLIKE_BUILTIN_CONSTANT_P */
> % 
> % -#else /* !(__GNUCLIKE_ASM && __GNUCLIKE_BUILTIN_CONSTANT_P) */
> % -
> % -/*
> % - * No optimizations are available for this compiler.  Fall back to
> % - * non-optimized functions by defining the constant usually used to prevent
> % - * redefinition.
> % - */
> % -#define	_BYTEORDER_FUNC_DEFINED
> % +/* XXX these are broken, since they evaluate x more than once. */

Maybe make them static inline functions. That won't be very efficient at
-O0 but it'll be correct and this implementation probably isn't very
efficient anyway. Otherwise the patch looks good to me.

> % +#define	__bswap16(x)	(__bswap16_gen((__uint16_t)(x)))
> % +#define	__bswap32(x)	(__bswap32_gen((__uint32_t)(x)))
> % +#define	__bswap64(x)	(__bswap64_gen((__uint64_t)(x)))
> % 
> %  #endif /* __GNUCLIKE_ASM && __GNUCLIKE_BUILTIN_CONSTANT_P */
> % 
> % -#ifdef __cplusplus
> % -}
> % -#endif
> % +#define	__htonl(x)	__bswap32(x)
> % +#define	__htons(x)	__bswap16(x)
> % +#define	__ntohl(x)	__bswap32(x)
> % +#define	__ntohs(x)	__bswap16(x)
> % 
> %  #endif /* !_MACHINE_ENDIAN_H_ */

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 228 bytes
Desc: This is a digitally signed message part.
Url : http://lists.freebsd.org/pipermail/svn-src-head/attachments/20120302/d79c7456/attachment.pgp