fix llrint and lrint for 52 <= exponent <= 62

Message ID 20180514133831.GA23372@mjk.surrey.ac.uk
State Accepted
Commit fcfea0ae2d213383f38b06690b6cf1454f2ac82d
Headers show
Series
  • fix llrint and lrint for 52 <= exponent <= 62
Related show

Commit Message

Matthias J. Kannwischer May 14, 2018, 1:38 p.m.
lrint and llrint produce wrong results if the exponent is between 52 and 62. 
e.g. llrint(2.308393893084546048E18) returns 146666059061806080 instead of 2308393893084546048. 
Can you confirm this?
I've tracked this down to two mistakes in s_llrint.c/s_lrint.c (one missing 0 and one uint32 overflow), see patch below. 

Cheers, 
Matthias

---
 newlib/libm/common/s_llrint.c | 4 ++--
 newlib/libm/common/s_lrint.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.14.3

Comments

Corinna Vinschen May 29, 2018, 1:25 p.m. | #1
Hi Matthias,

On May 14 14:38, Matthias J. Kannwischer wrote:
> lrint and llrint produce wrong results if the exponent is between 52 and 62. 

> e.g. llrint(2.308393893084546048E18) returns 146666059061806080 instead of 2308393893084546048. 

> Can you confirm this?

> I've tracked this down to two mistakes in s_llrint.c/s_lrint.c (one missing 0 and one uint32 overflow), see patch below. 

> 

> Cheers, 

> Matthias

> 

> ---

>  newlib/libm/common/s_llrint.c | 4 ++--

>  newlib/libm/common/s_lrint.c  | 4 ++--

>  2 files changed, 4 insertions(+), 4 deletions(-)

> 

> diff --git a/newlib/libm/common/s_llrint.c b/newlib/libm/common/s_llrint.c

> index 8b8a846ae..72452dbe9 100644

> --- a/newlib/libm/common/s_llrint.c

> +++ b/newlib/libm/common/s_llrint.c

> @@ -93,9 +93,9 @@ long long int

>        if (j0 >= 52)

>         /* 64bit return: j0 in [52,62] */

>         /* 64bit return: left shift amt in [32,42] */

> -        result = ((long long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) | 

> +        result = ((long long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |

>                 /* 64bit return: right shift amt in [0,10] */

> -                   (i1 << (j0 - 52));

> +                   ((long long int) i1 << (j0 - 52));

>        else

>          {

>           /* 64bit return: j0 in [20,51] */

> diff --git a/newlib/libm/common/s_lrint.c b/newlib/libm/common/s_lrint.c

> index 9d2cb7306..b37f50fd4 100644

> --- a/newlib/libm/common/s_lrint.c

> +++ b/newlib/libm/common/s_lrint.c

> @@ -131,9 +131,9 @@ TWO52[2]={

>        if (j0 >= 52)

>         /* 64bit return: j0 in [52,62] */

>         /* 64bit return: left shift amt in [32,42] */

> -        result = ((long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) | 

> +        result = ((long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |

>                 /* 64bit return: right shift amt in [0,10] */

> -                   (i1 << (j0 - 52));

> +                   ((long int) i1 << (j0 - 52));

>        else

>          {

>           /* 32bit return: j0 in [20,30] */

> -- 

> 2.14.3

> 


Looks right to me, but can you resend the patch as attachment, please?
Your MUA apparently replaced tabs with spaces.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Matthias J. Kannwischer May 29, 2018, 1:55 p.m. | #2
Hi Corinna, 

sure, it's attached - sorry for that. 

Best, 

Matthias 

On Tue, May 29, 2018 at 03:25:21PM +0200, Corinna Vinschen wrote:
> Hi Matthias,

> 

> On May 14 14:38, Matthias J. Kannwischer wrote:

> > lrint and llrint produce wrong results if the exponent is between 52 and 62. 

> > e.g. llrint(2.308393893084546048E18) returns 146666059061806080 instead of 2308393893084546048. 

> > Can you confirm this?

> > I've tracked this down to two mistakes in s_llrint.c/s_lrint.c (one missing 0 and one uint32 overflow), see patch below. 

> > 

> > Cheers, 

> > Matthias

> > 

> > ---

> >  newlib/libm/common/s_llrint.c | 4 ++--

> >  newlib/libm/common/s_lrint.c  | 4 ++--

> >  2 files changed, 4 insertions(+), 4 deletions(-)

> > 

> > diff --git a/newlib/libm/common/s_llrint.c b/newlib/libm/common/s_llrint.c

> > index 8b8a846ae..72452dbe9 100644

> > --- a/newlib/libm/common/s_llrint.c

> > +++ b/newlib/libm/common/s_llrint.c

> > @@ -93,9 +93,9 @@ long long int

> >        if (j0 >= 52)

> >         /* 64bit return: j0 in [52,62] */

> >         /* 64bit return: left shift amt in [32,42] */

> > -        result = ((long long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) | 

> > +        result = ((long long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |

> >                 /* 64bit return: right shift amt in [0,10] */

> > -                   (i1 << (j0 - 52));

> > +                   ((long long int) i1 << (j0 - 52));

> >        else

> >          {

> >           /* 64bit return: j0 in [20,51] */

> > diff --git a/newlib/libm/common/s_lrint.c b/newlib/libm/common/s_lrint.c

> > index 9d2cb7306..b37f50fd4 100644

> > --- a/newlib/libm/common/s_lrint.c

> > +++ b/newlib/libm/common/s_lrint.c

> > @@ -131,9 +131,9 @@ TWO52[2]={

> >        if (j0 >= 52)

> >         /* 64bit return: j0 in [52,62] */

> >         /* 64bit return: left shift amt in [32,42] */

> > -        result = ((long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) | 

> > +        result = ((long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |

> >                 /* 64bit return: right shift amt in [0,10] */

> > -                   (i1 << (j0 - 52));

> > +                   ((long int) i1 << (j0 - 52));

> >        else

> >          {

> >           /* 32bit return: j0 in [20,30] */

> > -- 

> > 2.14.3

> > 

> 

> Looks right to me, but can you resend the patch as attachment, please?

> Your MUA apparently replaced tabs with spaces.

> 

> 

> Thanks,

> Corinna

> 

> -- 

> Corinna Vinschen

> Cygwin Maintainer

> Red Hat
From f3d23be97ba6ec5aaad43aee6414cc51e7104116 Mon Sep 17 00:00:00 2001
From: Matthias Kannwischer <matthias@kannwischer.eu>

Date: Mon, 14 May 2018 14:00:18 +0100
Subject: [PATCH] fix llrint and lrint for 52 <= exponent <= 62

---
 newlib/libm/common/s_llrint.c | 4 ++--
 newlib/libm/common/s_lrint.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/newlib/libm/common/s_llrint.c b/newlib/libm/common/s_llrint.c
index 8b8a846ae..72452dbe9 100644
--- a/newlib/libm/common/s_llrint.c
+++ b/newlib/libm/common/s_llrint.c
@@ -93,9 +93,9 @@ long long int
       if (j0 >= 52)
 	/* 64bit return: j0 in [52,62] */
 	/* 64bit return: left shift amt in [32,42] */
-        result = ((long long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) | 
+        result = ((long long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |
 		/* 64bit return: right shift amt in [0,10] */
-                   (i1 << (j0 - 52));
+                   ((long long int) i1 << (j0 - 52));
       else
         {
 	  /* 64bit return: j0 in [20,51] */
diff --git a/newlib/libm/common/s_lrint.c b/newlib/libm/common/s_lrint.c
index 9d2cb7306..b37f50fd4 100644
--- a/newlib/libm/common/s_lrint.c
+++ b/newlib/libm/common/s_lrint.c
@@ -131,9 +131,9 @@ TWO52[2]={
       if (j0 >= 52)
 	/* 64bit return: j0 in [52,62] */
 	/* 64bit return: left shift amt in [32,42] */
-        result = ((long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) | 
+        result = ((long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |
 		/* 64bit return: right shift amt in [0,10] */
-                   (i1 << (j0 - 52));
+                   ((long int) i1 << (j0 - 52));
       else
         {
 	  /* 32bit return: j0 in [20,30] */
-- 
2.14.3
Corinna Vinschen May 29, 2018, 2:02 p.m. | #3
On May 29 14:55, Matthias J. Kannwischer wrote:
> Hi Corinna, 

> 

> sure, it's attached - sorry for that. 

> 

> Best, 

> 

> Matthias 

> 

> On Tue, May 29, 2018 at 03:25:21PM +0200, Corinna Vinschen wrote:

> > Hi Matthias,

> > 

> > On May 14 14:38, Matthias J. Kannwischer wrote:

> > > lrint and llrint produce wrong results if the exponent is between 52 and 62. 

> > > e.g. llrint(2.308393893084546048E18) returns 146666059061806080 instead of 2308393893084546048. 

> > > Can you confirm this?

> > > I've tracked this down to two mistakes in s_llrint.c/s_lrint.c (one missing 0 and one uint32 overflow), see patch below. 

> > > 

> > > Cheers, 

> > > Matthias

> > > [...]

> > 

> > Looks right to me, but can you resend the patch as attachment, please?

> > Your MUA apparently replaced tabs with spaces.

> > 

> > 

> > Thanks,

> > Corinna


Pushed.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Patch

diff --git a/newlib/libm/common/s_llrint.c b/newlib/libm/common/s_llrint.c
index 8b8a846ae..72452dbe9 100644
--- a/newlib/libm/common/s_llrint.c
+++ b/newlib/libm/common/s_llrint.c
@@ -93,9 +93,9 @@  long long int
       if (j0 >= 52)
        /* 64bit return: j0 in [52,62] */
        /* 64bit return: left shift amt in [32,42] */
-        result = ((long long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) | 
+        result = ((long long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |
                /* 64bit return: right shift amt in [0,10] */
-                   (i1 << (j0 - 52));
+                   ((long long int) i1 << (j0 - 52));
       else
         {
          /* 64bit return: j0 in [20,51] */
diff --git a/newlib/libm/common/s_lrint.c b/newlib/libm/common/s_lrint.c
index 9d2cb7306..b37f50fd4 100644
--- a/newlib/libm/common/s_lrint.c
+++ b/newlib/libm/common/s_lrint.c
@@ -131,9 +131,9 @@  TWO52[2]={
       if (j0 >= 52)
        /* 64bit return: j0 in [52,62] */
        /* 64bit return: left shift amt in [32,42] */
-        result = ((long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) | 
+        result = ((long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |
                /* 64bit return: right shift amt in [0,10] */
-                   (i1 << (j0 - 52));
+                   ((long int) i1 << (j0 - 52));
       else
         {
          /* 32bit return: j0 in [20,30] */