Date: Thu, 1 Jul 2010 05:17:49 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Marcel Moolenaar <xcllnt@mac.com> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, Marcel Moolenaar <marcel@FreeBSD.org>, Bruce Evans <brde@optusnet.com.au>, src-committers@FreeBSD.org Subject: Re: svn commit: r209604 - head/lib/libc/gmon Message-ID: <20100701050124.J4356@besplex.bde.org> In-Reply-To: <79DD181D-3885-45F5-9E9D-753553D19891@mac.com> References: <201006300140.o5U1eQVG097566@svn.freebsd.org> <20100630184517.B51465@delplex.bde.org> <79DD181D-3885-45F5-9E9D-753553D19891@mac.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 30 Jun 2010, Marcel Moolenaar wrote:
> On Jun 30, 2010, at 2:40 AM, Bruce Evans wrote:
>
>> On Wed, 30 Jun 2010, Marcel Moolenaar wrote:
>>
>>> Log:
>>> On powerpc, calculate s_scale using the non-FP version previously
>>> specific to hp300. Since FreeBSD does not support hp300, hp300 has
>>> been removed from the condition altogether.
>>
>> Also, the style of the condition has been regressed from ifndef to
>> if !defined().
>
> That's on purpose. Either we eliminate the whole conditional or we
> end up adding other platforms to it.
Better to inhibit such mistakes. It's almost a sign of a mistake that
there is a long list of platforms in an ifdef.
> ...
>> Better still, rewrite the integer method using a C99 type, so that it
>> is as simple as the FP method:
>>
>> s_scale = ((uintmax_t)p->kcountsize << SCALE_SHIFT) / o;
>>
>> C99 uintmax_t now guarantees uintmax_t to have >= 64 bits, and practical
>> considerations guarantee p->kcountsize to fit in many fewer than 48 bits
>> even on 64-bit arches, so the shift cannot overflow.
>
> I like this. What about the following (white-space corrupted)
> simplification:
Almost OK. I orginally wrote it with a conditional operator, but decided
that that might be harder to understand.
> Index: gmon.c
> ===================================================================
> --- gmon.c (revision 209604)
> +++ gmon.c (working copy)
> @@ -110,24 +110,9 @@
> p->tos[0].link = 0;
>
> o = p->highpc - p->lowpc;
> - if (p->kcountsize < o) {
> -#if !defined(__powerpc__)
> - s_scale = ((float)p->kcountsize / o ) * SCALE_1_TO_1;
> -#else /* avoid floating point */
> - int quot = o / p->kcountsize;
> + s_scale = (p->kcountsize < o) ?
> + ((uintmax_t)p->kcountsize << SCALE_1_TO_1) / o : SCALE_1_TO_1;
Shifting by 65536 is a bit much :-). Multiplication by 65536 might
give the same code as shifting by 16, but I think shifting is clearer.
The kernel uses shifting for the reverse conversion, with `16' hard-coded
in 2 statements and 2 comments, and with too much duplication in the
comments.
>
Remove this blank line too.
> - if (quot >= 0x10000)
> - s_scale = 1;
> ...
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100701050124.J4356>