Date: Fri, 1 Mar 2013 15:15:43 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Alexander Motin <mav@freebsd.org> Cc: Davide Italiano <davide@freebsd.org>, svn-src-head@freebsd.org, Alexey Dokuchaev <danfe@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org Subject: Re: svn commit: r247460 - head/sys/dev/acpica Message-ID: <20130301151538.G959@besplex.bde.org> In-Reply-To: <512F95DC.1040005@FreeBSD.org> References: <201302281127.r1SBR2VE068276@svn.freebsd.org> <20130228162522.GA41693@FreeBSD.org> <512F95DC.1040005@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130301151538.G959>