From owner-svn-src-all@FreeBSD.ORG Wed Feb 29 01:56:24 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 19206106566B; Wed, 29 Feb 2012 01:56:24 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail08.syd.optusnet.com.au (mail08.syd.optusnet.com.au [211.29.132.189]) by mx1.freebsd.org (Postfix) with ESMTP id A359D8FC0A; Wed, 29 Feb 2012 01:56:23 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q1T1uLqB006250 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 29 Feb 2012 12:56:22 +1100 Date: Wed, 29 Feb 2012 12:56:21 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans In-Reply-To: <20120229105104.K1479@besplex.bde.org> Message-ID: <20120229115001.F1479@besplex.bde.org> References: <201202281939.q1SJdtYr084858@svn.freebsd.org> <20120229105104.K1479@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, Tijl Coosemans , 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 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Feb 2012 01:56:24 -0000 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