Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 29 Feb 2012 12:56:21 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Bruce Evans <brde@optusnet.com.au>
Cc:        svn-src-head@freebsd.org, Tijl Coosemans <tijl@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org
Subject:   Re: svn commit: r232266 - in head/sys: amd64/include i386/include pc98/include x86/include
Message-ID:  <20120229115001.F1479@besplex.bde.org>
In-Reply-To: <20120229105104.K1479@besplex.bde.org>
References:  <201202281939.q1SJdtYr084858@svn.freebsd.org> <20120229105104.K1479@besplex.bde.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 29 Feb 2012, Bruce Evans wrote:

> On Tue, 28 Feb 2012, Tijl Coosemans wrote:
>
> The patch is unreadable due to significant reimplementations, even without
> the move.  So I looked at the resulting x86/endian.h:

Oops.  I somehow looked at the old i386 version.  The committed version
is mostly OK, and fixes or avoids most of the bugs that I pointed out:

% #ifdef __cplusplus
% extern "C" {
% #endif

Still have this.

% #define	__bswap16_const(_x)	(__uint16_t)((_x) << 8 | (_x) >> 8)
% 
% #define	__bswap16(_x)			\
% 	(__builtin_constant_p(_x) ?	\
% 	    __bswap16_const((__uint16_t)(_x)) : __bswap16_var(_x))

There seems to be no need for the const and var cases for 16 bits (use
the expression for the const case unconditionally, with more casts in
it).

We haven't remove the bogus underscores in macro parameter names yet.

% 
% #define	__bswap32_const(_x)		\
% 	(((__uint32_t)__bswap16(_x) << 16) | __bswap16((_x) >> 16))

Now uses "building-up".

% 
% #define	__bswap32(_x)			\
% 	(__builtin_constant_p(_x) ?	\
% 	    __bswap32_const((__uint32_t)(_x)) : __bswap32_var(_x))

The technical considerations described in my previous mail prevent
killing the separate cases and using pure building up from 16 bits to
32 and 64 bits.

This is quite confusing.  __bswap32_const() is perfectly general since
it uses the general __bswap16() macros, but it is spelled with a `const'.
OTOH, __bswap32() is perfectly general by name and functionality, but is
only useful for the `var' case.

% ...
% static __inline __uint16_t
% __bswap16_var(__uint16_t _x)
% {
% 
% 	return (__bswap16_const(_x));
% }

Not needed, since we killed the asm for the 16-bit case.

% 
% static __inline __uint32_t
% __bswap32_var(__uint32_t _x)
% {
% 
% 	__asm ("bswap %0" : "+r" (_x));
% 	return (_x);
% }
% 
% static __inline __uint64_t
% __bswap64_var(__uint64_t _x)
% {
% #ifdef	_LP64

Tab instead of space after ifdef.

I think it is gcc and not _LP64 that needs this help.

% 	__asm ("bswap %0" : "+r" (_x));
% 	return (_x);
% #else
% 	return (__bswap64_const(_x));

The use of `const' here confused me into thinking that this still uses
the old slow `const' code.

% #endif
% }
% ...
% #ifdef __cplusplus
% }
% #endif

Still have this.

The best cleanups seem to be:
- use general building up with no asms and no inline functions for the
   default case (put this in a MI header)
- use asms for gcc.  I think the following works for minimizing the
   ifdefs for this:
   - start with bswap32(), etc., defined directly to the default using
     building up (I'm trying to avoid having the __bswap32() layer too)
   - to override this, just #undef it and redefine it with a gcc asm in
     it (it seems best to use a statement expression and not an inline)
   or:
   - start with __bswap32() defined to the default using building up
   - normally define bswap32 to __bswap32.  Otherwise, define bswap32
     to our special asm or C implementation.
   or:
   - some combination of the above.  We can use __bswap32 in an inline
     function with the same name, and then #undef the macro:

% #define	__bswap32(x)	(...)
% 
% int
% (__bswap32)(int _x)
% {
% #ifdef asm_case
% 	{( asm(...
% #else
% 	return (__bswap32(_x));
% #endif
% }
% 
% #undef __bswap32

     This technique permits the default macros to use the best names and
     not require adorning them with _const or _var or maybe even __ and
     then needing more layers to #define them back to the best names.

     I don't know how to do this without turning the macro into an inline
     function.  There is related magic for macros that I forget.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120229115001.F1479>