Adjust complex sqrt in compiler to use the C functions #24231
Conversation
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
mppf
force-pushed
the
follow-24127-libc++
branch
from
e573a88
to
c2f0424
Compare
mppf
changed the title
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
| @@ -18,10 +18,14 @@ | |||
| target_sources(ChplFrontend-obj | |||
| PRIVATE | |||
|
|
|||
| ifa_vars.cpp | |||
| # C sources | |||
| complex-support.h | |||
There was a problem hiding this comment.
Huh -- do header files usually get listed in sources? I don't recall that to be the case, but I could be forgetting.
There was a problem hiding this comment.
We have been listing them inconsistently. E.g. in frontend/lib/util/CMakeLists.txt. AFAIK it is harmless but unnecessary to list them.
| #include <complex.h> | ||
|
|
||
| static double complex makeDoubleComplex(double re, double im) { | ||
| // Some test environments don't have a working CMPLXF |
There was a problem hiding this comment.
should this say "working CMPLX" (no F?)
| #define cmplx_re64(c) (((double *)&(c))[0]) | ||
| #define cmplx_im64(c) (((double *)&(c))[1]) | ||
| double complex val; | ||
| cmplx_re64(val) = re; | ||
| cmplx_im64(val) = im; |
There was a problem hiding this comment.
At this point, I would do away with the #defines and simply include the original expressions + a comment ("set the real part" .e.g.).
| #define cmplx_re64(c) (((double *)&(c))[0]) | |
| #define cmplx_im64(c) (((double *)&(c))[1]) | |
| double complex val; | |
| cmplx_re64(val) = re; | |
| cmplx_im64(val) = im; | |
| double complex val; | |
| (((double *)&val)[0]) = re; /* set the real part of the complex number */ | |
| (((double *)&val)[1]) = im; /* set the imaginary part of the complex number */ |
There was a problem hiding this comment.
Since this code is pretty much the same as _chpl_complex128 etc that Brad added to chpltypes.h in the runtime in #24184 / #24211 I wanted to follow the same pattern. I agree that the macro is not strictly necessary here, but it is arguably valuable to communicate intent -- which you handled with the comments.
@bradcray how would you feel about removing the macros in both of these places?
There was a problem hiding this comment.
As someone who habitually doesn't read comments (like, I didn't even see them here until I saw Michael's "which you handled with the comments" and prefers self-descriptive code, I'd probably still stick with the macros, personally. But I'm admittedly less invested in the frontend/ directory than I am the runtime, so don't have a preference for this PR. Just not sure I'd remove them from the runtime for the sake of consistency.
| #ifndef CHPL_DONT_USE_CMPLX_PTR_ALIASING | ||
| #define cmplx_re32(c) (((float *)&(c))[0]) | ||
| #define cmplx_im32(c) (((float *)&(c))[1]) | ||
| float complex val; | ||
| cmplx_re32(val) = re; | ||
| cmplx_im32(val) = im; |
There was a problem hiding this comment.
if you take my prior suggestion, this is another place to apply it
| struct complex64 ret; | ||
| ret.r = crealf(n); | ||
| ret.i = cimagf(n); |
There was a problem hiding this comment.
Since we're in C, could you simply write:
return { .r = crealf(n), .i = cimagf(n) };Or perhaps:
struct complex64 ret = { .r = crealf(n), .i = cimagf(n) };| struct complex128 ret; | ||
| ret.r = creal(n); | ||
| ret.i = cimag(n); |
There was a problem hiding this comment.
ditto my previous comment.
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
Follow-up to PR #24127.
After #24127, the new
sqrtparamstest started to fail on Mac OS X system. Upon investigating, I identified that the difference in behavior from what the test expects is due to differences in howstd::sqrtis implemented for complex numbers in libc++ vs stdlibc++. Also, I noticed that the issue is limited to the C++ implementation of the complex square root.This PR changes the implementation to use a C implementation of complex square root since it appears to have more reliable precision. Note that this C implementation required some workarounds for
CMPLXnot being available on all systems (as with PR #24184 and #24211). In particular,CMPLXdoes not seem to be available when usingclangon Ubuntu 23.10, even though it is available withgccon that system.I filed an issue against libc++ about the problem, but in discussion there it sounds like complex
sqrtisn't required to have any particular precision.Reviewed by @DanilaFe - thanks!