Date: Mon, 9 Dec 2013 23:38:40 -0800 From: Steve Kargl <sgk@troutmask.apl.washington.edu> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-numerics@freebsd.org, Filipe Maia <filipe.c.maia@gmail.com> Subject: Re: dead code in lgamma_r[f]? Message-ID: <20131210073840.GA94002@troutmask.apl.washington.edu> In-Reply-To: <20131210155635.J1022@besplex.bde.org> References: <20131205195225.GA65732@troutmask.apl.washington.edu> <20131206102724.W1033@besplex.bde.org> <20131206114426.I1329@besplex.bde.org> <20131207064602.GA76042@troutmask.apl.washington.edu> <20131208012939.GA80145@troutmask.apl.washington.edu> <20131208141627.F1181@besplex.bde.org> <20131208183339.GA83010@troutmask.apl.washington.edu> <20131208185941.GA83484@troutmask.apl.washington.edu> <20131209223720.GA91828@troutmask.apl.washington.edu> <20131210155635.J1022@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Dec 10, 2013 at 04:26:12PM +1100, Bruce Evans wrote:
> On Mon, 9 Dec 2013, Steve Kargl wrote:
>
> > I would like to commit the attached patch. I will do it in
> > 3 passes:
> > 1) whitespace fixes,
> > 2) literal constants changes (checked by md5),
> > 3) the code re-organization, threshold change, and dead code removed.
> >
> > * lib/msun/src/e_lgamma_r.c
> > . Remove trailing space and trailing blank line in Copyright.
>
> This undoes rev.1.8, in which previous trailing whitespace removal in
> 1.2 was backed out to reduce diffs against the vendor version.
It reduces the diff between e_lgamma_r.c and e_lgammaf.c. In addition,
I think that at this point in time, the original vendor is irrelevant.
>From what I can glean from OpenBSD, NetBSD, Openlibm, and dragonflybsd,
FreeBSD is the vendor.
> > . Fix prototype for sin_pi to agree with the intended commit
> > r97413 done some 11 years 6 month ago.
> > . Remove dead code in sin_pi and remove 'if(ix<0x43300000)' as it is
> > always true.
> > . In the domain [0,2), move the three minimax approximations embedded
> > within __ieee75_lgamma_r() into three 'static inline double' functions.
>
> This is too invasive. It is only a style change to a slighly worse style.
It's only a style change to a much better style in that I now
have some idea of what to do get working ld80 and ld128 versions.
>
> The other (unrelated) changes seem reasonable.
>
> > --- /usr/src/lib/msun/src/e_lgamma_r.c 2013-12-06 07:58:39.000000000 -0800
> > +++ src/e_lgamma_r.c 2013-12-09 13:05:22.000000000 -0800
> > ...
> > double
> > __ieee754_lgamma_r(double x, int *signgamp)
> > {
> > - double t,y,z,nadj,p,p1,p2,p3,q,r,w;
> > + double t,y,z,nadj,p,q,r,w;
> > int32_t hx;
> > - int i,lx,ix;
> > + int i,ix,lx;
>
> Also avoid fixing sorting errors when not changing much. It would be a
> more substantive change to fix the types of these variables (they should
> be uint32_t or int32_t). I should have done more or less in rev.1.9
> where I split up this declaration to change only hx from int to int32_t.
>
> >
> > EXTRACT_WORDS(hx,lx,x);
> >
> > @@ -235,51 +265,36 @@
> > if((((ix-0x3ff00000)|lx)==0)||(((ix-0x40000000)|lx)==0)) r = 0;
> > /* for x < 2.0 */
> > else if(ix<0x40000000) {
> > - if(ix<=0x3feccccc) { /* lgamma(x) = lgamma(x+1)-log(x) */
> > + if (ix <= 0X3FECCCCC) { /* lgamma(x) = lgamma(x+1)-log(x) */
>
> Like whitespace fixes.
>
> > r = -__ieee754_log(x);
> > - if(ix>=0x3FE76944) {y = one-x; i= 0;}
> > - else if(ix>=0x3FCDA661) {y= x-(tc-one); i=1;}
> > - else {y = x; i=2;}
> > + if (ix >= 0x3FE76944)
> > + r += func0(1 - x);
> > + else if (ix >= 0x3FCDA661)
> > + r += func1(x - (tc - 1));
> > + else
> > + r += func2(x);
>
> The simplification from using func*() seems to be mainly avoiding a case
> statement and the case selector variable i.
It also allows one to clearly see that here func[0-2] are
evaluations of lgamma(1+x) where subdomains in [0,2) are
being mapped into other domains.
> > ...
> > switch(i) {
> > - case 7: z *= (y+6.0); /* FALLTHRU */
> > - case 6: z *= (y+5.0); /* FALLTHRU */
> > - case 5: z *= (y+4.0); /* FALLTHRU */
> > - case 4: z *= (y+3.0); /* FALLTHRU */
> > - case 3: z *= (y+2.0); /* FALLTHRU */
> > + case 7: z *= (y+6); /* FALLTHRU */
> > + case 6: z *= (y+5); /* FALLTHRU */
> > + case 5: z *= (y+4); /* FALLTHRU */
> > + case 4: z *= (y+3); /* FALLTHRU */
> > + case 3: z *= (y+2); /* FALLTHRU */
>
> The comment indentation would be wrong now, except this file already uses
> totally inconsistent comment indentation. The float version blindly moved
> the indentation in the other direction by adding float casts.
The change reduces the diff between float and double versions.
--
Steve
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131210073840.GA94002>