Skip site navigation (1)Skip section navigation (2)
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: <http://docs.FreeBSD.org/cgi/mid.cgi?20131210073840.GA94002>