mailing list of musl libc
 help / color / mirror / code / Atom feed
* Patch for cacosh
@ 2019-10-01 21:57 Michael Morrell
  2019-10-01 23:28 ` Rich Felker
  2019-10-02 11:49 ` Szabolcs Nagy
  0 siblings, 2 replies; 4+ messages in thread
From: Michael Morrell @ 2019-10-01 21:57 UTC (permalink / raw)
  To: musl


[-- Attachment #1.1: Type: text/plain, Size: 293 bytes --]

Running the gcc validation suite, I noticed that gfortran.dg/complex_intrinsic_5.f90 was failing when using MUSL.

I tracked it down to the cacosh routines not getting the correct result when the imaginary part of the argument was negative.

Attached is a patch to fix this.

  Michael

[-- Attachment #1.2: Type: text/html, Size: 2182 bytes --]

[-- Attachment #2: cacosh.patch --]
[-- Type: application/octet-stream, Size: 1279 bytes --]

diff --git a/src/complex/cacosh.c b/src/complex/cacosh.c
index 8c68cb01..0b598052 100644
--- a/src/complex/cacosh.c
+++ b/src/complex/cacosh.c
@@ -4,6 +4,9 @@
 
 double complex cacosh(double complex z)
 {
+        _Bool zineg = cimag(z) < 0;
+
 	z = cacos(z);
-	return CMPLX(-cimag(z), creal(z));
+	if (zineg) return CMPLX(cimag(z), -creal(z));
+	else       return CMPLX(-cimag(z), creal(z));
 }
diff --git a/src/complex/cacoshf.c b/src/complex/cacoshf.c
index ade01c09..82e6c751 100644
--- a/src/complex/cacoshf.c
+++ b/src/complex/cacoshf.c
@@ -2,6 +2,9 @@
 
 float complex cacoshf(float complex z)
 {
+	_Bool zineg = cimagf(z) < 0;
+
 	z = cacosf(z);
-	return CMPLXF(-cimagf(z), crealf(z));
+	if (zineg) return CMPLXF(cimagf(z), -crealf(z));
+	else       return CMPLXF(-cimagf(z), crealf(z));
 }
diff --git a/src/complex/cacoshl.c b/src/complex/cacoshl.c
index 65342557..b0081e90 100644
--- a/src/complex/cacoshl.c
+++ b/src/complex/cacoshl.c
@@ -8,7 +8,10 @@ long double complex cacoshl(long double complex z)
 #else
 long double complex cacoshl(long double complex z)
 {
+	_Bool zineg = cimagl(z) < 0;
+
 	z = cacosl(z);
-	return CMPLXL(-cimagl(z), creall(z));
+	if (zineg) return CMPLXL(cimagl(z), -creall(z));
+	else       return CMPLXL(-cimagl(z), creall(z));
 }
 #endif

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Patch for cacosh
  2019-10-01 21:57 Patch for cacosh Michael Morrell
@ 2019-10-01 23:28 ` Rich Felker
  2019-10-02 16:45   ` Michael Morrell
  2019-10-02 11:49 ` Szabolcs Nagy
  1 sibling, 1 reply; 4+ messages in thread
From: Rich Felker @ 2019-10-01 23:28 UTC (permalink / raw)
  To: musl

On Tue, Oct 01, 2019 at 09:57:17PM +0000, Michael Morrell wrote:
> Running the gcc validation suite, I noticed that
> gfortran.dg/complex_intrinsic_5.f90 was failing when using MUSL.
> 
> I tracked it down to the cacosh routines not getting the correct
> result when the imaginary part of the argument was negative.
> 
> Attached is a patch to fix this.

Thanks!

> diff --git a/src/complex/cacosh.c b/src/complex/cacosh.c
> index 8c68cb01..0b598052 100644
> --- a/src/complex/cacosh.c
> +++ b/src/complex/cacosh.c
> @@ -4,6 +4,9 @@
>  
>  double complex cacosh(double complex z)
>  {
> +        _Bool zineg = cimag(z) < 0;
> +

I think this fails to give the desired result for negative zero.
Should probably be signbit(cimag(z)) or similar.

If there's a way to adjust the input to cacos to avoid having to patch
up after it returns based on a flag saved before the call, that would
make it more efficient I think (no need to spill/reload), but it's not
a big deal if not.

Also, as a style matter, musl codebase generally doesn't use _Bool;
rather just int for flags/boolean values.

Rich


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Patch for cacosh
  2019-10-01 21:57 Patch for cacosh Michael Morrell
  2019-10-01 23:28 ` Rich Felker
@ 2019-10-02 11:49 ` Szabolcs Nagy
  1 sibling, 0 replies; 4+ messages in thread
From: Szabolcs Nagy @ 2019-10-02 11:49 UTC (permalink / raw)
  To: musl

* Michael Morrell <mmorrell@tachyum.com> [2019-10-01 21:57:17 +0000]:
> Running the gcc validation suite, I noticed that gfortran.dg/complex_intrinsic_5.f90 was failing when using MUSL.
> 
> I tracked it down to the cacosh routines not getting the correct result when the imaginary part of the argument was negative.

yeah the complex functions are not expected to be correct:
i made no attempt to get all principal values right nor
to deal with fp format special cases (rounding, overflow,..)

i suspect that even with the sign check all sorts of
special cases are wrong (input with large real or imag part),
but if it helps somewhere then i think it's ok to add the
patch (it should just be clear that the implementation
is not expected to be correct, fixing up complex would
be a huge amount of work)

> 
> Attached is a patch to fix this.
> 
>   Michael



^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: Patch for cacosh
  2019-10-01 23:28 ` Rich Felker
@ 2019-10-02 16:45   ` Michael Morrell
  0 siblings, 0 replies; 4+ messages in thread
From: Michael Morrell @ 2019-10-02 16:45 UTC (permalink / raw)
  To: musl

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]

Here's a revised version that used "int" instead of "_Bool" and signbit().  You were correct that -0 failed.

I'm not sure how to do this by adjusting the input to cacos, so I left that alone.

Thanks for considering taking this patch.   Most of the rest of the failures in the Fortran portion of the gcc validation suite are due to lack of quad math support which I know is something you want to tackle, but is still a ways off.

   Michael

-----Original Message-----
From: Rich Felker <dalias@aerifal.cx> On Behalf Of Rich Felker
Sent: Tuesday, October 1, 2019 4:28 PM
To: musl@lists.openwall.com
Subject: Re: [musl] Patch for cacosh

On Tue, Oct 01, 2019 at 09:57:17PM +0000, Michael Morrell wrote:
> Running the gcc validation suite, I noticed that
> gfortran.dg/complex_intrinsic_5.f90 was failing when using MUSL.
> 
> I tracked it down to the cacosh routines not getting the correct 
> result when the imaginary part of the argument was negative.
> 
> Attached is a patch to fix this.

Thanks!

> diff --git a/src/complex/cacosh.c b/src/complex/cacosh.c index 
> 8c68cb01..0b598052 100644
> --- a/src/complex/cacosh.c
> +++ b/src/complex/cacosh.c
> @@ -4,6 +4,9 @@
>  
>  double complex cacosh(double complex z)  {
> +        _Bool zineg = cimag(z) < 0;
> +

I think this fails to give the desired result for negative zero.
Should probably be signbit(cimag(z)) or similar.

If there's a way to adjust the input to cacos to avoid having to patch up after it returns based on a flag saved before the call, that would make it more efficient I think (no need to spill/reload), but it's not a big deal if not.

Also, as a style matter, musl codebase generally doesn't use _Bool; rather just int for flags/boolean values.

Rich

[-- Attachment #2: cacosh.patch --]
[-- Type: application/octet-stream, Size: 1281 bytes --]

diff --git a/src/complex/cacosh.c b/src/complex/cacosh.c
index 8e42f1ae..76127f75 100644
--- a/src/complex/cacosh.c
+++ b/src/complex/cacosh.c
@@ -4,6 +4,9 @@
 
 double complex cacosh(double complex z)
 {
+	int zineg = signbit(cimag(z));
+
 	z = cacos(z);
-	return CMPLX(-cimag(z), creal(z));
+	if (zineg) return CMPLX(cimag(z), -creal(z));
+	else       return CMPLX(-cimag(z), creal(z));
 }
diff --git a/src/complex/cacoshf.c b/src/complex/cacoshf.c
index d7e6b545..8bd80581 100644
--- a/src/complex/cacoshf.c
+++ b/src/complex/cacoshf.c
@@ -2,6 +2,9 @@
 
 float complex cacoshf(float complex z)
 {
+	int zineg = signbit(cimagf(z));
+
 	z = cacosf(z);
-	return CMPLXF(-cimagf(z), crealf(z));
+	if (zineg) return CMPLXF(cimagf(z), -crealf(z));
+	else       return CMPLXF(-cimagf(z), crealf(z));
 }
diff --git a/src/complex/cacoshl.c b/src/complex/cacoshl.c
index d3eaee20..3a284be9 100644
--- a/src/complex/cacoshl.c
+++ b/src/complex/cacoshl.c
@@ -8,7 +8,10 @@ long double complex cacoshl(long double complex z)
 #else
 long double complex cacoshl(long double complex z)
 {
+	int zineg = signbit(cimagl(z));
+
 	z = cacosl(z);
-	return CMPLXL(-cimagl(z), creall(z));
+	if (zineg) return CMPLXL(cimagl(z), -creall(z));
+	else       return CMPLXL(-cimagl(z), creall(z));
 }
 #endif

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-10-02 16:45 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-01 21:57 Patch for cacosh Michael Morrell
2019-10-01 23:28 ` Rich Felker
2019-10-02 16:45   ` Michael Morrell
2019-10-02 11:49 ` Szabolcs Nagy

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).