From owner-svn-src-head@FreeBSD.ORG Fri Mar 1 04:16:04 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id AEECF31A; Fri, 1 Mar 2013 04:16:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx07.syd.optusnet.com.au (fallbackmx07.syd.optusnet.com.au [211.29.132.9]) by mx1.freebsd.org (Postfix) with ESMTP id E05326A3; Fri, 1 Mar 2013 04:16:03 +0000 (UTC) Received: from mail28.syd.optusnet.com.au (mail28.syd.optusnet.com.au [211.29.133.169]) by fallbackmx07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r214FvXH023251; Fri, 1 Mar 2013 15:15:57 +1100 Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail28.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r214Fh0f022953 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 1 Mar 2013 15:15:44 +1100 Date: Fri, 1 Mar 2013 15:15:43 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Alexander Motin Subject: Re: svn commit: r247460 - head/sys/dev/acpica In-Reply-To: <512F95DC.1040005@FreeBSD.org> Message-ID: <20130301151538.G959@besplex.bde.org> References: <201302281127.r1SBR2VE068276@svn.freebsd.org> <20130228162522.GA41693@FreeBSD.org> <512F95DC.1040005@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.0 cv=bNdOu4CZ c=1 sm=1 a=f5xiIF61AdoA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=7E17uGdFteIA:10 a=6I5d2MoRAAAA:8 a=ba2XAj07wnl6R86JeWQA:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: Davide Italiano , svn-src-head@freebsd.org, Alexey Dokuchaev , src-committers@freebsd.org, svn-src-all@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Mar 2013 04:16:04 -0000 On Thu, 28 Feb 2013, Alexander Motin wrote: > On 28.02.2013 18:25, Alexey Dokuchaev wrote: >> On Thu, Feb 28, 2013 at 11:27:02AM +0000, Davide Italiano wrote: >>> New Revision: 247460 >>> URL: http://svnweb.freebsd.org/changeset/base/247460 >>> >>> Log: >>> MFcalloutng (r247427 by mav): >>> We don't need any precision here. Let it be fast and dirty shift then >>> slow and excessively precise 64-bit division. >>> >>> - if (sbt >= 0 && us > sbt / SBT_1US) >>> - us = sbt / SBT_1US; >>> + if (sbt >= 0 && us > (sbt >> 12)) >>> + us = (sbt >> 12); >> >> Does this really buy us anything? Modern compilers should be smart enough to >> generate correct code. Do you have evidence that this is not the case here? >> Not to mention that it obfuscates the code by using some magic constant. > > SBT_1US is 4294 (0x10c6). The best that compiler may do is replace > division with multiplication. In fact, Clang even does this on amd64. > But on i386 it calls __divdi3(), doing 64bit division in software. Shift > is definitely cheaper and 5% precision is fine here. I missed the additional magic in my previous reply. But you should write the sloppy scaling as division by a sloppy factor: #define SSBT_1us 4096 /* power of 2 closest to SSBT_1US */ if (sbt >= 0 && us > (uint64_t)sbt / SSBT_1us) us = (uint64_t)sbt / SSBT_1us; or provide and use conversion functions that do sloppy and non-sloppy scaling. I don't like having conversion functions for every possible conversion, but this one is much more magic than for example TIMEVAL_TO_TIMESPEC(). The casts to (uint64_t) are to help the compiler understand that the sign bit is not there. The need for magic scaling shows that the binary representation given by sbintime_t isn't very good. Mose clients want natural units of microseconds or nanoseconds and need scale factors like (4294.967206 / 4096) to adjust (4294 is already sloppy). The binary representation allows some minor internal optimizations and APIs are made unnatural to avoid double conversions. While here, I will point out style bugs introduced in the above: - parentheses in "us = (sbt >> 12);" are redundant and reduce clarity, like parentheses in "us = (sbt / N);" would have, since the shift operator binds much more tightly than the assignment operator. - parentheses in "us > (sbt >> 12);" are redundant but may increase clarity, since the shift operator doesn't bind much more tightly than the '<' comparison operator. This one is hard to remember, but looking it up confirms that the precedence is not broken as designed in this case, but that the precedence is only 1 level higher for the shift operator. The main broken as designed cases are the shift operator being 1 level lower than addition and subtraction, and bitwise operators being many more levels lower than other aritmetic operators and even below all comparision operators. Bruce