[PATCH] Fix make check build warnings in libm

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix make check build warnings in libm

Siddhesh Poyarekar-3
Hi,

The patch below fixes a few build warnings in libm tests:

atest-sincos.c: In function ‘sincosx_mpn’:
atest-sincos.c:67:19: warning: variable ‘round’ set but not used [-Wunused-but-set-variable]
atest-exp.c: In function ‘exp_mpn’:
atest-exp.c:64:19: warning: variable ‘round’ set but not used [-Wunused-but-set-variable]
atest-exp2.c: In function ‘exp_mpn’:
atest-exp2.c:105:19: warning: variable ‘round’ set but not used [-Wunused-but-set-variable]

OK to commit?

Siddhesh

        * math/atest-exp.c (exp_mpn): Remove ROUND.
        * math/atest-exp2.c (exp_mpn): Likewise.
        * math/atest-sincos.c (sincosx_mpn): Likewise.

diff --git a/math/atest-exp.c b/math/atest-exp.c
index 2678e74..d76b912 100644
--- a/math/atest-exp.c
+++ b/math/atest-exp.c
@@ -61,7 +61,7 @@ exp_mpn (mp1 ex, mp1 x)
    unsigned n;
    mp1 xp;
    mp2 tmp;
-   mp_limb_t chk, round;
+   mp_limb_t chk;
    mp1 tol;
 
    memset (xp, 0, sizeof (mp1));
@@ -79,7 +79,7 @@ exp_mpn (mp1 ex, mp1 x)
        mpn_mul_n (tmp, xp, x, SZ);
        assert (tmp[SZ * 2 - 1] == 0);
        if (n > 0)
- round = mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
+ mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
        chk = mpn_add_n (ex, ex, xp, SZ);
        assert (chk == 0);
        n++;
diff --git a/math/atest-exp2.c b/math/atest-exp2.c
index b05d43b..20836ca 100644
--- a/math/atest-exp2.c
+++ b/math/atest-exp2.c
@@ -102,7 +102,7 @@ exp_mpn (mp1 ex, mp1 x)
    unsigned int n;
    mp1 xp;
    mp2 tmp;
-   mp_limb_t chk, round;
+   mp_limb_t chk;
    mp1 tol;
 
    memset (xp, 0, sizeof (mp1));
@@ -120,7 +120,7 @@ exp_mpn (mp1 ex, mp1 x)
        mpn_mul_n (tmp, xp, x, SZ);
        assert(tmp[SZ * 2 - 1] == 0);
        if (n > 0)
- round = mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
+ mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
        chk = mpn_add_n (ex, ex, xp, SZ);
        assert (chk == 0);
        ++n;
diff --git a/math/atest-sincos.c b/math/atest-sincos.c
index bea157c..3b61760 100644
--- a/math/atest-sincos.c
+++ b/math/atest-sincos.c
@@ -64,7 +64,7 @@ sincosx_mpn (mp1 si, mp1 co, mp1 xx, mp1 ix)
    int i;
    mp2 s[4], c[4];
    mp1 tmp, x;
-   mp_limb_t chk, round;
+   mp_limb_t chk;
 
    if (ix == NULL)
      {
@@ -103,7 +103,7 @@ sincosx_mpn (mp1 si, mp1 co, mp1 xx, mp1 ix)
  mpn_lshift(tmp,tmp,SZ,1);      \
  chk |= mpn_add_n(tmp,tmp,s[0]+FRAC/mpbpl,SZ);      \
  chk |= mpn_add_n(tmp,tmp,s[3]+FRAC/mpbpl,SZ);      \
- round = mpn_divmod_1(tmp,tmp,SZ,6);      \
+ mpn_divmod_1(tmp,tmp,SZ,6);      \
  /* chk |= mpn_add_1(tmp,tmp,SZ, (round > 3) ); */      \
          chk |= (n ? mpn_sub_n : mpn_add_n)(d,ss,tmp,SZ);      \
  /* assert(chk == 0); */      \
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix make check build warnings in libm

Carlos O'Donell-2
On Fri, Feb 15, 2013 at 7:16 AM, Siddhesh Poyarekar <[hidden email]> wrote:

> Hi,
>
> The patch below fixes a few build warnings in libm tests:
>
> atest-sincos.c: In function ‘sincosx_mpn’:
> atest-sincos.c:67:19: warning: variable ‘round’ set but not used [-Wunused-but-set-variable]
> atest-exp.c: In function ‘exp_mpn’:
> atest-exp.c:64:19: warning: variable ‘round’ set but not used [-Wunused-but-set-variable]
> atest-exp2.c: In function ‘exp_mpn’:
> atest-exp2.c:105:19: warning: variable ‘round’ set but not used [-Wunused-but-set-variable]
>
> OK to commit?
>
> Siddhesh
>
>         * math/atest-exp.c (exp_mpn): Remove ROUND.
>         * math/atest-exp2.c (exp_mpn): Likewise.
>         * math/atest-sincos.c (sincosx_mpn): Likewise.
>
> diff --git a/math/atest-exp.c b/math/atest-exp.c
> index 2678e74..d76b912 100644
> --- a/math/atest-exp.c
> +++ b/math/atest-exp.c
> @@ -61,7 +61,7 @@ exp_mpn (mp1 ex, mp1 x)
>     unsigned n;
>     mp1 xp;
>     mp2 tmp;
> -   mp_limb_t chk, round;
> +   mp_limb_t chk;
>     mp1 tol;
>
>     memset (xp, 0, sizeof (mp1));
> @@ -79,7 +79,7 @@ exp_mpn (mp1 ex, mp1 x)
>         mpn_mul_n (tmp, xp, x, SZ);
>         assert (tmp[SZ * 2 - 1] == 0);
>         if (n > 0)
> -        round = mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
> +        mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
>         chk = mpn_add_n (ex, ex, xp, SZ);
>         assert (chk == 0);
>         n++;

OK.

> diff --git a/math/atest-exp2.c b/math/atest-exp2.c
> index b05d43b..20836ca 100644
> --- a/math/atest-exp2.c
> +++ b/math/atest-exp2.c
> @@ -102,7 +102,7 @@ exp_mpn (mp1 ex, mp1 x)
>     unsigned int n;
>     mp1 xp;
>     mp2 tmp;
> -   mp_limb_t chk, round;
> +   mp_limb_t chk;
>     mp1 tol;
>
>     memset (xp, 0, sizeof (mp1));
> @@ -120,7 +120,7 @@ exp_mpn (mp1 ex, mp1 x)
>         mpn_mul_n (tmp, xp, x, SZ);
>         assert(tmp[SZ * 2 - 1] == 0);
>         if (n > 0)
> -        round = mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
> +        mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
>         chk = mpn_add_n (ex, ex, xp, SZ);
>         assert (chk == 0);
>         ++n;

OK.

> diff --git a/math/atest-sincos.c b/math/atest-sincos.c
> index bea157c..3b61760 100644
> --- a/math/atest-sincos.c
> +++ b/math/atest-sincos.c
> @@ -64,7 +64,7 @@ sincosx_mpn (mp1 si, mp1 co, mp1 xx, mp1 ix)
>     int i;
>     mp2 s[4], c[4];
>     mp1 tmp, x;
> -   mp_limb_t chk, round;
> +   mp_limb_t chk;
>
>     if (ix == NULL)
>       {
> @@ -103,7 +103,7 @@ sincosx_mpn (mp1 si, mp1 co, mp1 xx, mp1 ix)
>          mpn_lshift(tmp,tmp,SZ,1);                                            \
>          chk |= mpn_add_n(tmp,tmp,s[0]+FRAC/mpbpl,SZ);                        \
>          chk |= mpn_add_n(tmp,tmp,s[3]+FRAC/mpbpl,SZ);                        \
> -        round = mpn_divmod_1(tmp,tmp,SZ,6);                                  \
> +        mpn_divmod_1(tmp,tmp,SZ,6);                                          \
>          /* chk |= mpn_add_1(tmp,tmp,SZ, (round > 3) ); */                    \

What about this commented out line?

Why are we using chk if all the assert's on chk are commented out?

>           chk |= (n ? mpn_sub_n : mpn_add_n)(d,ss,tmp,SZ);                    \
>          /* assert(chk == 0); */                                              \

Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix make check build warnings in libm

Siddhesh Poyarekar-3
On Fri, Feb 15, 2013 at 09:44:34PM -0500, Carlos O'Donell wrote:
>
> What about this commented out line?
>
> Why are we using chk if all the assert's on chk are commented out?
>
> >           chk |= (n ? mpn_sub_n : mpn_add_n)(d,ss,tmp,SZ);                    \
> >          /* assert(chk == 0); */                                              \
>

I'm not sure why, but most mpn functions seem to return carry or
borrow if any or just zero.  It's been that way since inclusion
(1997), so it might make sense to just remove them.  I don't think any
of the inputs will cause an overflow and the checks were probably
there earlier for debugging.

Siddhesh
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix make check build warnings in libm

Carlos O'Donell-2
On Sun, Feb 17, 2013 at 10:42 PM, Siddhesh Poyarekar
<[hidden email]> wrote:

> On Fri, Feb 15, 2013 at 09:44:34PM -0500, Carlos O'Donell wrote:
>>
>> What about this commented out line?
>>
>> Why are we using chk if all the assert's on chk are commented out?
>>
>> >           chk |= (n ? mpn_sub_n : mpn_add_n)(d,ss,tmp,SZ);                    \
>> >          /* assert(chk == 0); */                                              \
>>
>
> I'm not sure why, but most mpn functions seem to return carry or
> borrow if any or just zero.  It's been that way since inclusion
> (1997), so it might make sense to just remove them.  I don't think any
> of the inputs will cause an overflow and the checks were probably
> there earlier for debugging.

Right, but this function doesn't return anything :-)

Debugging checks should be actively compiled into the code and
removed by the compiler when a conditional statement is easily
provably to be false.

This is what GCC does to avoid bitrot in the debugging and testing
code, and it's one thing I've seen the community mention was great
at keeping this code updated.

I'd like to see this file 100% fixed, not 50% fixed.

Can you look at removing chk? We can always go back to an old
version later with source control, but right now it's not useful to have
code for chk that does nothing.

Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix make check build warnings in libm

Siddhesh Poyarekar-3
On Mon, Feb 18, 2013 at 10:38:18AM -0500, Carlos O'Donell wrote:
> Can you look at removing chk? We can always go back to an old
> version later with source control, but right now it's not useful to have
> code for chk that does nothing.

OK, here's v2 then:

        * math/atest-exp.c (exp_mpn): Remove ROUND.
        * math/atest-exp2.c (exp_mpn): Likewise.
        * math/atest-sincos.c (sincosx_mpn): Remove ROUND and CHK.

diff --git a/math/atest-exp.c b/math/atest-exp.c
index 2678e74..d76b912 100644
--- a/math/atest-exp.c
+++ b/math/atest-exp.c
@@ -61,7 +61,7 @@ exp_mpn (mp1 ex, mp1 x)
    unsigned n;
    mp1 xp;
    mp2 tmp;
-   mp_limb_t chk, round;
+   mp_limb_t chk;
    mp1 tol;
 
    memset (xp, 0, sizeof (mp1));
@@ -79,7 +79,7 @@ exp_mpn (mp1 ex, mp1 x)
        mpn_mul_n (tmp, xp, x, SZ);
        assert (tmp[SZ * 2 - 1] == 0);
        if (n > 0)
- round = mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
+ mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
        chk = mpn_add_n (ex, ex, xp, SZ);
        assert (chk == 0);
        n++;
diff --git a/math/atest-exp2.c b/math/atest-exp2.c
index b05d43b..20836ca 100644
--- a/math/atest-exp2.c
+++ b/math/atest-exp2.c
@@ -102,7 +102,7 @@ exp_mpn (mp1 ex, mp1 x)
    unsigned int n;
    mp1 xp;
    mp2 tmp;
-   mp_limb_t chk, round;
+   mp_limb_t chk;
    mp1 tol;
 
    memset (xp, 0, sizeof (mp1));
@@ -120,7 +120,7 @@ exp_mpn (mp1 ex, mp1 x)
        mpn_mul_n (tmp, xp, x, SZ);
        assert(tmp[SZ * 2 - 1] == 0);
        if (n > 0)
- round = mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
+ mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
        chk = mpn_add_n (ex, ex, xp, SZ);
        assert (chk == 0);
        ++n;
diff --git a/math/atest-sincos.c b/math/atest-sincos.c
index bea157c..313bccb 100644
--- a/math/atest-sincos.c
+++ b/math/atest-sincos.c
@@ -64,7 +64,6 @@ sincosx_mpn (mp1 si, mp1 co, mp1 xx, mp1 ix)
    int i;
    mp2 s[4], c[4];
    mp1 tmp, x;
-   mp_limb_t chk, round;
 
    if (ix == NULL)
      {
@@ -79,34 +78,38 @@ sincosx_mpn (mp1 si, mp1 co, mp1 xx, mp1 ix)
    for (i = 0; i < 1 << N; i++)
      {
 #define add_shift_mulh(d,x,s1,s2,sh,n) \
-       /* d = (n ? -1 : 1) * (s1 + (s2>>sh)) * x / (1>>N); */      \
       do {      \
  if (s2 != NULL) {      \
     if (sh > 0) {      \
        assert (sh < mpbpl);      \
        mpn_lshift (tmp, s1, SZ, sh);      \
-       chk = (n ? mpn_sub_n : mpn_add_n)(tmp,tmp,s2+FRAC/mpbpl,SZ);   \
-    } else      \
-       chk = (n ? mpn_sub_n : mpn_add_n)(tmp,s1,s2+FRAC/mpbpl,SZ);    \
-    /* assert(chk == 0); */      \
+       if (n)      \
+         mpn_sub_n (tmp,tmp,s2+FRAC/mpbpl,SZ);      \
+       else      \
+         mpn_add_n (tmp,tmp,s2+FRAC/mpbpl,SZ);      \
+    } else {      \
+       if (n)      \
+         mpn_sub_n (tmp,s1,s2+FRAC/mpbpl,SZ);      \
+       else      \
+         mpn_add_n (tmp,s1,s2+FRAC/mpbpl,SZ);      \
+    }      \
     mpn_mul_n(d,tmp,x,SZ);      \
  } else      \
     mpn_mul_n(d,s1,x,SZ);      \
- /* assert(d[SZ*2-1] == 0); */      \
  assert(N+sh < mpbpl);      \
  if (N+sh > 0) mpn_rshift(d,d,2*SZ,N+sh);      \
       } while(0)
 #define summ(d,ss,s,n) \
-      /* d = ss +/- (s[0]+2*s[1]+2*s[2]+s[3])/6; */      \
       do {      \
- chk = mpn_add_n(tmp,s[1]+FRAC/mpbpl,s[2]+FRAC/mpbpl,SZ);      \
+ mpn_add_n(tmp,s[1]+FRAC/mpbpl,s[2]+FRAC/mpbpl,SZ);      \
  mpn_lshift(tmp,tmp,SZ,1);      \
- chk |= mpn_add_n(tmp,tmp,s[0]+FRAC/mpbpl,SZ);      \
- chk |= mpn_add_n(tmp,tmp,s[3]+FRAC/mpbpl,SZ);      \
- round = mpn_divmod_1(tmp,tmp,SZ,6);      \
- /* chk |= mpn_add_1(tmp,tmp,SZ, (round > 3) ); */      \
-         chk |= (n ? mpn_sub_n : mpn_add_n)(d,ss,tmp,SZ);      \
- /* assert(chk == 0); */      \
+ mpn_add_n(tmp,tmp,s[0]+FRAC/mpbpl,SZ);      \
+ mpn_add_n(tmp,tmp,s[3]+FRAC/mpbpl,SZ);      \
+ mpn_divmod_1(tmp,tmp,SZ,6);      \
+ if (n)      \
+           mpn_sub_n (d,ss,tmp,SZ);      \
+ else      \
+           mpn_add_n (d,ss,tmp,SZ);      \
       } while (0)
 
       add_shift_mulh (s[0], x, co, NULL, 0, 0); /* s0 = h * c; */
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix make check build warnings in libm

Carlos O'Donell-2
On Tue, Feb 19, 2013 at 6:44 AM, Siddhesh Poyarekar <[hidden email]> wrote:

> OK, here's v2 then:
>
>         * math/atest-exp.c (exp_mpn): Remove ROUND.
>         * math/atest-exp2.c (exp_mpn): Likewise.
>         * math/atest-sincos.c (sincosx_mpn): Remove ROUND and CHK.
>
> diff --git a/math/atest-exp.c b/math/atest-exp.c
> index 2678e74..d76b912 100644
> --- a/math/atest-exp.c
> +++ b/math/atest-exp.c
> @@ -61,7 +61,7 @@ exp_mpn (mp1 ex, mp1 x)
>     unsigned n;
>     mp1 xp;
>     mp2 tmp;
> -   mp_limb_t chk, round;
> +   mp_limb_t chk;
>     mp1 tol;
>
>     memset (xp, 0, sizeof (mp1));
> @@ -79,7 +79,7 @@ exp_mpn (mp1 ex, mp1 x)
>         mpn_mul_n (tmp, xp, x, SZ);
>         assert (tmp[SZ * 2 - 1] == 0);
>         if (n > 0)
> -        round = mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
> +        mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
>         chk = mpn_add_n (ex, ex, xp, SZ);
>         assert (chk == 0);
>         n++;
> diff --git a/math/atest-exp2.c b/math/atest-exp2.c
> index b05d43b..20836ca 100644
> --- a/math/atest-exp2.c
> +++ b/math/atest-exp2.c
> @@ -102,7 +102,7 @@ exp_mpn (mp1 ex, mp1 x)
>     unsigned int n;
>     mp1 xp;
>     mp2 tmp;
> -   mp_limb_t chk, round;
> +   mp_limb_t chk;
>     mp1 tol;
>
>     memset (xp, 0, sizeof (mp1));
> @@ -120,7 +120,7 @@ exp_mpn (mp1 ex, mp1 x)
>         mpn_mul_n (tmp, xp, x, SZ);
>         assert(tmp[SZ * 2 - 1] == 0);
>         if (n > 0)
> -        round = mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
> +        mpn_divmod_1 (xp, tmp + FRAC / mpbpl, SZ, n);
>         chk = mpn_add_n (ex, ex, xp, SZ);
>         assert (chk == 0);
>         ++n;
> diff --git a/math/atest-sincos.c b/math/atest-sincos.c
> index bea157c..313bccb 100644
> --- a/math/atest-sincos.c
> +++ b/math/atest-sincos.c
> @@ -64,7 +64,6 @@ sincosx_mpn (mp1 si, mp1 co, mp1 xx, mp1 ix)
>     int i;
>     mp2 s[4], c[4];
>     mp1 tmp, x;
> -   mp_limb_t chk, round;
>
>     if (ix == NULL)
>       {
> @@ -79,34 +78,38 @@ sincosx_mpn (mp1 si, mp1 co, mp1 xx, mp1 ix)
>     for (i = 0; i < 1 << N; i++)
>       {
>  #define add_shift_mulh(d,x,s1,s2,sh,n) \
> -       /* d = (n ? -1 : 1) * (s1 + (s2>>sh)) * x / (1>>N); */                \
>        do {                                                                   \
>          if (s2 != NULL) {                                                    \
>             if (sh > 0) {                                                     \
>                assert (sh < mpbpl);                                           \
>                mpn_lshift (tmp, s1, SZ, sh);                                  \
> -              chk = (n ? mpn_sub_n : mpn_add_n)(tmp,tmp,s2+FRAC/mpbpl,SZ);   \
> -           } else                                                            \
> -              chk = (n ? mpn_sub_n : mpn_add_n)(tmp,s1,s2+FRAC/mpbpl,SZ);    \
> -           /* assert(chk == 0); */                                           \
> +              if (n)                                                         \
> +                mpn_sub_n (tmp,tmp,s2+FRAC/mpbpl,SZ);                        \
> +              else                                                           \
> +                mpn_add_n (tmp,tmp,s2+FRAC/mpbpl,SZ);                        \
> +           } else {                                                          \
> +              if (n)                                                         \
> +                mpn_sub_n (tmp,s1,s2+FRAC/mpbpl,SZ);                         \
> +              else                                                           \
> +                mpn_add_n (tmp,s1,s2+FRAC/mpbpl,SZ);                         \
> +           }                                                                 \
>             mpn_mul_n(d,tmp,x,SZ);                                            \
>          } else                                                               \
>             mpn_mul_n(d,s1,x,SZ);                                             \
> -        /* assert(d[SZ*2-1] == 0); */                                        \
>          assert(N+sh < mpbpl);                                                \
>          if (N+sh > 0) mpn_rshift(d,d,2*SZ,N+sh);                             \
>        } while(0)
>  #define summ(d,ss,s,n) \
> -      /* d = ss +/- (s[0]+2*s[1]+2*s[2]+s[3])/6; */                          \
>        do {                                                                   \
> -        chk = mpn_add_n(tmp,s[1]+FRAC/mpbpl,s[2]+FRAC/mpbpl,SZ);             \
> +        mpn_add_n(tmp,s[1]+FRAC/mpbpl,s[2]+FRAC/mpbpl,SZ);                   \
>          mpn_lshift(tmp,tmp,SZ,1);                                            \
> -        chk |= mpn_add_n(tmp,tmp,s[0]+FRAC/mpbpl,SZ);                        \
> -        chk |= mpn_add_n(tmp,tmp,s[3]+FRAC/mpbpl,SZ);                        \
> -        round = mpn_divmod_1(tmp,tmp,SZ,6);                                  \
> -        /* chk |= mpn_add_1(tmp,tmp,SZ, (round > 3) ); */                    \
> -         chk |= (n ? mpn_sub_n : mpn_add_n)(d,ss,tmp,SZ);                    \
> -        /* assert(chk == 0); */                                              \
> +        mpn_add_n(tmp,tmp,s[0]+FRAC/mpbpl,SZ);                               \
> +        mpn_add_n(tmp,tmp,s[3]+FRAC/mpbpl,SZ);                               \
> +        mpn_divmod_1(tmp,tmp,SZ,6);                                          \
> +        if (n)                                                               \
> +           mpn_sub_n (d,ss,tmp,SZ);                                          \
> +        else                                                                 \
> +           mpn_add_n (d,ss,tmp,SZ);                                          \
>        } while (0)
>
>        add_shift_mulh (s[0], x, co, NULL, 0, 0); /* s0 = h * c; */

This looks good to me.

Cheers,
Carlos.