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>
index | next in thread | previous in thread | raw e-mail
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. Brucehome | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130301151538.G959>