svn commit: r247460 - head/sys/dev/acpica

[Bruce Evans]

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