dead code in lgamma_r[f]?

Bruce Evans brde at optusnet.com.au
Tue Dec 10 05:26:27 UTC 2013


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.

>  . 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.

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.

> ...
> 	    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.

Bruce


More information about the freebsd-numerics mailing list