This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Consolidate and inline calls to __acr


On 12/31/2012 08:47 AM, Siddhesh Poyarekar wrote:
> Hi,
> 
> The __acr function is a simple comparison between the absolute values
> of two mp numbers.  It is not used anywhere outside of the base
> operations, so I have made it a static internal function.  The
> performance improvement is not much (approx. 0.2%) with the current
> code, but it gets better once the mantissa gets converted to int
> (approx 0.5%).  However, I see this patch more as cleanup than as
> being of any significant value in terms of performance.  I have
> verified that the testsuite does not regress as a result of this
> change.  OK to commit?

No.

The mcr removal should be a distinct patch (and come first).

The cleanup should be a distinct patch (and second).

The __acr change should be a distinct patch (and final).

We should all be in the habit of using commits to make small
changes that support reg-testing for other targets or compiler
problems.

You can list all three commits in one email if you want
showing the logical progression.

I'll even bet that you did all three steps above while testing
your patch :-)

> Siddhesh
> 
> ChangeLog:
> 
> 	* sysdeps/ieee754/dbl-64/mpa.c (mcr): Remove.
> 	(__acr): Make static inline.
> 	(__add): Clean up and consolidate calls to __acr.
> 	(__sub): Likewise.
> 	* sysdeps/ieee754/dbl-64/mpa.h (__acr): Remove declaration.
> 
> diff --git a/sysdeps/ieee754/dbl-64/mpa.c b/sysdeps/ieee754/dbl-64/mpa.c
> index e608cf1..26b2632 100644
> --- a/sysdeps/ieee754/dbl-64/mpa.c
> +++ b/sysdeps/ieee754/dbl-64/mpa.c
> @@ -56,41 +56,33 @@ const mp_no mpone = {1, {1.0, 1.0}};
>  const mp_no mptwo = {1, {1.0, 2.0}};
>  #endif
>  
> -#ifndef NO___ACR

You don't explain why you're allowed to remove the conditional?

> -/* mcr() compares the sizes of the mantissas of two multiple precision  */
> -/* numbers. Mantissas are compared regardless of the signs of the       */
> -/* numbers, even if x->d[0] or y->d[0] are zero. Exponents are also     */
> -/* disregarded.                                                         */
> -static int
> -mcr(const mp_no *x, const mp_no *y, int p) {
> -  int i;
> -  for (i=1; i<=p; i++) {
> -    if      (X[i] == Y[i])  continue;
> -    else if (X[i] >  Y[i])  return  1;
> -    else                    return -1; }
> -  return 0;
> -}

Inlining of mcr into __acr looks OK.

> -
> -
> -/* acr() compares the absolute values of two multiple precision numbers */
> -int
> -__acr(const mp_no *x, const mp_no *y, int p) {
> -  int i;
> -
> -  if      (X[0] == ZERO) {
> -    if    (Y[0] == ZERO) i= 0;
> -    else                 i=-1;
> -  }
> -  else if (Y[0] == ZERO) i= 1;
> -  else {
> -    if      (EX >  EY)   i= 1;
> -    else if (EX <  EY)   i=-1;
> -    else                 i= mcr(x,y,p);
> -  }
> +/* Compare the absolute values of two multiple precision numbers.  */
> +static inline int
> +__acr (const mp_no *x, const mp_no *y, int p)
> +{
> +  if (X[0] == ZERO || Y[0] == ZERO)
> +    return (int) (X[0] - Y[0]);
>  
> -  return i;
> +  if (EX >  EY)
> +    return 1;
> +  else if (EX <  EY)
> +    return -1;
> +  else
> +    {
> +      /* Mantissa may be different.  */
> +      int i;
> +      for (i=1; i<=p; i++)
> +        {
> +	  if (X[i] < Y[i])
> +	    return -1;
> +          else if (X[i] >  Y[i])
> +	    return  1;
> +	  else
> +	    continue;
> +	}
> +    }
> +  return 0;
>  }
> -#endif

How much bigger does __acr get?

>  
>  #if 0
> @@ -394,22 +386,49 @@ sub_magnitudes(const mp_no *x, const mp_no *y, mp_no *z, int p) {
>  
>  void
>  SECTION
> -__add(const mp_no *x, const mp_no *y, mp_no *z, int p) {
> -
> -  int n;
> +__add(const mp_no *x, const mp_no *y, mp_no *z, int p)
> +{
> +  if (X[0] == ZERO)
> +    {
> +      __cpy(y,z,p);
> +      return;
> +    }
> +  else if (Y[0] == ZERO)
> +    {
> +      __cpy(x,z,p);
> +      return;
> +    }
>  
> -  if      (X[0] == ZERO)     {__cpy(y,z,p);  return; }
> -  else if (Y[0] == ZERO)     {__cpy(x,z,p);  return; }
> +  int n = __acr (x, y, p);
>  
> -  if (X[0] == Y[0])   {
> -    if (__acr(x,y,p) > 0)      {add_magnitudes(x,y,z,p);  Z[0] = X[0]; }
> -    else                     {add_magnitudes(y,x,z,p);  Z[0] = Y[0]; }
> -  }
> -  else                       {
> -    if ((n=__acr(x,y,p)) == 1) {sub_magnitudes(x,y,z,p);  Z[0] = X[0]; }
> -    else if (n == -1)        {sub_magnitudes(y,x,z,p);  Z[0] = Y[0]; }
> -    else                      Z[0] = ZERO;
> -  }
> +  if (X[0] == Y[0])
> +    {
> +      if (n == 1)
> +	{
> +	  add_magnitudes(x,y,z,p);
> +	  Z[0] = X[0];
> +	}
> +      else
> +	{
> +	  add_magnitudes(y,x,z,p);
> +	  Z[0] = Y[0];
> +	}
> +    }
> +  else
> +    {
> +      if (n == 1)
> +	{
> +	  sub_magnitudes(x,y,z,p);
> +	  Z[0] = X[0];
> +	}
> +      else if (n == -1)
> +	{
> +	  sub_magnitudes(y,x,z,p);
> +	  Z[0] = Y[0];
> +	}
> +      else
> +	Z[0] = ZERO;
> +    }
>  }

You've rearranged the checks into a more logical ordering IMO
(e.g. exit early first), but I'm curious is this impacts the 
code size by much?

Change is OK.

>  
> @@ -419,22 +438,50 @@ __add(const mp_no *x, const mp_no *y, mp_no *z, int p) {
>  
>  void
>  SECTION
> -__sub(const mp_no *x, const mp_no *y, mp_no *z, int p) {
> -
> -  int n;
> +__sub(const mp_no *x, const mp_no *y, mp_no *z, int p)
> +{
> +  if (X[0] == ZERO)
> +    {
> +      __cpy(y,z,p);
> +      Z[0] = -Z[0];
> +      return;
> +    }
> +  else if (Y[0] == ZERO)
> +    {
> +      __cpy(x,z,p);
> +      return;
> +    }
>  
> -  if      (X[0] == ZERO)     {__cpy(y,z,p);  Z[0] = -Z[0];  return; }
> -  else if (Y[0] == ZERO)     {__cpy(x,z,p);                 return; }
> +  int n = __acr (x, y, p);
>  
> -  if (X[0] != Y[0])    {
> -    if (__acr(x,y,p) > 0)      {add_magnitudes(x,y,z,p);  Z[0] =  X[0]; }
> -    else                     {add_magnitudes(y,x,z,p);  Z[0] = -Y[0]; }
> -  }
> -  else                       {
> -    if ((n=__acr(x,y,p)) == 1) {sub_magnitudes(x,y,z,p);  Z[0] =  X[0]; }
> -    else if (n == -1)        {sub_magnitudes(y,x,z,p);  Z[0] = -Y[0]; }
> -    else                      Z[0] = ZERO;
> -  }
> +  if (X[0] != Y[0])
> +    {
> +      if (n == 1)
> +	{
> +	  add_magnitudes(x,y,z,p);
> +	  Z[0] =  X[0];
> +	}
> +      else
> +	{
> +	  add_magnitudes(y,x,z,p);
> +	  Z[0] = -Y[0];
> +	}
> +    }
> +  else
> +    {
> +      if (n == 1)
> +	{
> +	  sub_magnitudes(x,y,z,p);
> +	  Z[0] =  X[0];
> +	}
> +      else if (n == -1)
> +	{
> +	  sub_magnitudes(y,x,z,p);
> +	  Z[0] = -Y[0];
> +	}
> +      else
> +	Z[0] = ZERO;
> +    }
>  }

Same question about function code size.

Change looks OK.

> diff --git a/sysdeps/ieee754/dbl-64/mpa.h b/sysdeps/ieee754/dbl-64/mpa.h
> index 2a2deb4..da6db31 100644
> --- a/sysdeps/ieee754/dbl-64/mpa.h
> +++ b/sysdeps/ieee754/dbl-64/mpa.h
> @@ -66,7 +66,6 @@ extern const mp_no mptwo;
>  
>  #define ABS(x)   ((x) <  0  ? -(x) : (x))
>  
> -int __acr(const mp_no *, const mp_no *, int);
>  // int  __cr(const mp_no *, const mp_no *, int);
>  void __cpy(const mp_no *, mp_no *, int);
>  // void __cpymn(const mp_no *, int, mp_no *, int);
> 

This is because the function isn't used outside of mpa.c
so therefore doesn't need to be in a header?

Should we not adjust the header comment to match reality?

e.g.
diff --git a/sysdeps/ieee754/dbl-64/mpa.h b/sysdeps/ieee754/dbl-64/mpa.h
index 26ae2ac..a222d54 100644
--- a/sysdeps/ieee754/dbl-64/mpa.h
+++ b/sysdeps/ieee754/dbl-64/mpa.h
@@ -21,18 +21,13 @@
 /*  MODULE_NAME: mpa.h                                                  */
 /*                                                                      */
 /*  FUNCTIONS:                                                          */
-/*               mcr                                                    */
-/*               acr                                                    */
-/*               cr                                                     */
 /*               cpy                                                    */
-/*               cpymn                                                  */
 /*               mp_dbl                                                 */
 /*               dbl_mp                                                 */
 /*               add                                                    */
 /*               sub                                                    */
 /*               mul                                                    */
 /*               inv                                                    */
-/*               dvd                                                    */
 /*                                                                      */
 /* Arithmetic functions for multiple precision numbers.                 */
 /* Common types and definition                                          */
@@ -63,16 +58,12 @@ typedef union { int i[2]; double d; } number;
 
 #define ABS(x)   ((x) <  0  ? -(x) : (x))
 
-int __acr(const mp_no *, const mp_no *, int);
-// int  __cr(const mp_no *, const mp_no *, int);
 void __cpy(const mp_no *, mp_no *, int);
-// void __cpymn(const mp_no *, int, mp_no *, int);
 void __mp_dbl(const mp_no *, double *, int);
 void __dbl_mp(double, mp_no *, int);
 void __add(const mp_no *, const mp_no *, mp_no *, int);
 void __sub(const mp_no *, const mp_no *, mp_no *, int);
 void __mul(const mp_no *, const mp_no *, mp_no *, int);
-// void __inv(const mp_no *, mp_no *, int);
 void __dvd(const mp_no *, const mp_no *, mp_no *, int);
 
 extern void __mpatan (mp_no *, mp_no *, int);
---

Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]