Patch for cacosh

Patch for cacosh

[Michael Morrell]

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

Re: Patch for cacosh

[Rich Felker]

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

RE: Patch for cacosh

[Michael Morrell]

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

Re: Patch for cacosh

[Szabolcs Nagy]

* 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