From owner-svn-src-all@freebsd.org Mon May 30 03:09:53 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 87A29B53500; Mon, 30 May 2016 03:09:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id 18B8B1ABC; Mon, 30 May 2016 03:09:52 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c110-21-42-169.carlnfd1.nsw.optusnet.com.au [110.21.42.169]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id 075A9D42F26; Mon, 30 May 2016 13:09:28 +1000 (AEST) Date: Mon, 30 May 2016 13:09:28 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Conrad Meyer cc: "Andrey A. Chernov" , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r300965 - head/lib/libc/stdlib In-Reply-To: Message-ID: <20160530122100.X924@besplex.bde.org> References: <201605291639.u4TGdSwq032144@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=c+ZWOkJl c=1 sm=1 tr=0 a=kDyANCGC9fy361NNEb9EQQ==:117 a=kDyANCGC9fy361NNEb9EQQ==:17 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=mDnqfD46IR0aj72RSGUA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 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: Mon, 30 May 2016 03:09:53 -0000 On Sun, 29 May 2016, Conrad Meyer wrote: > Does clang actually generate different code with this change? It should, on exotic arches. > On Sun, May 29, 2016 at 9:39 AM, Andrey A. Chernov wrote: >> Log: >> Micro optimize: C standard guarantees that right shift for unsigned value >> fills left bits with zero, and we have exact 32bit unsigned value >> (uint32_t), so there is no reason to add "& 0x7fffffff" here. Using uint32_t at all is an unportable pessimization. On exotic arches that don't have native uint32_t registers, a theoretically perfect implementation wouldn't implement uint32_t. This would expose the brokenness of broken code, so a practical implementation would emulate uint32_t so that the broken code would just run slower for arithmetic and much slower for locked memory operations. uint32_t might be implemented not very slowly using 128-bit integer registers, or more slowly using the 53-bit mantissa part of an IEEE double precision floating point register. If uint32_t is emulated, then the compiler is forced to act as if the code uses a longer type and does "& 0xffffffff" after every operation. Thes extra operations can only be combined sometimes. More careful code can use a minimal number of this or similar "&" operations. In checksum calculations, one "&" at the end is usually enough. >> Modified: head/lib/libc/stdlib/random.c random.c was mostly written before uint32_t was standard, so it used u_long and long. Perhaps it wasn't careful enough with the "&"s to actually work unless u_long is precisely uint32_t and long is normal 2's complement with benign overflow. Anyway, it was "fixed" (unimproved) using s/u_long/uint32_/ in most places where the API/ABI doesn't require longs (there is 1 dubious long left in a comment). The correct fix is s/u_long/uint_fast32_t in most places and s/u_long/uint_least32_t/ in some places and then fix any missing "&"'s. The "fast" and "least" types always exist, unlike the fixed-width types, and using them asks for time/space efficiency instead of emulated fixed-width. On non-exotic arches, fast == least == fixed-width, so the correct substitution works as a quick fix even with missing "&"s. It is not necessary to use the newfangled standard integer types to fix this here, since correct use of long types would work (they give 32 bits), but long is wasteful if it actually 64 bits or longer. Even larger problems are looming with uintmax_t. Any code that is careful enough to use it is likely to break or be bloated if it is expanded. This is just like using u_long in old random(). >> ============================================================================== >> --- head/lib/libc/stdlib/random.c Sun May 29 16:32:56 2016 (r300964) >> +++ head/lib/libc/stdlib/random.c Sun May 29 16:39:28 2016 (r300965) >> @@ -430,7 +430,7 @@ random(void) >> */ >> f = fptr; r = rptr; >> *f += *r; >> - i = (*f >> 1) & 0x7fffffff; /* chucking least random bit */ >> + i = *f >> 1; /* chucking least random bit */ This gives an "&" to restore in the version with correct substitutions. It also breaks the indentation. (This file mostly indents comments to the right of code to column 40, but column 48 was used here and now column 32 is used.) >> if (++f >= end_ptr) { >> f = state; >> ++r; Bruce