From owner-svn-src-all@FreeBSD.ORG Wed Jun 30 19:17:53 2010 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6FB0E1065670; Wed, 30 Jun 2010 19:17:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail02.syd.optusnet.com.au (mail02.syd.optusnet.com.au [211.29.132.183]) by mx1.freebsd.org (Postfix) with ESMTP id 03FA98FC13; Wed, 30 Jun 2010 19:17:52 +0000 (UTC) Received: from besplex.bde.org (c122-106-145-25.carlnfd1.nsw.optusnet.com.au [122.106.145.25]) by mail02.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o5UJHnx8001986 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 1 Jul 2010 05:17:50 +1000 Date: Thu, 1 Jul 2010 05:17:49 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Marcel Moolenaar In-Reply-To: <79DD181D-3885-45F5-9E9D-753553D19891@mac.com> Message-ID: <20100701050124.J4356@besplex.bde.org> References: <201006300140.o5U1eQVG097566@svn.freebsd.org> <20100630184517.B51465@delplex.bde.org> <79DD181D-3885-45F5-9E9D-753553D19891@mac.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, Marcel Moolenaar , Bruce Evans , src-committers@FreeBSD.org Subject: Re: svn commit: r209604 - head/lib/libc/gmon X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 Jun 2010 19:17:53 -0000 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