[Einstein Toolkit] #1534: Bug in complex numbers

List overview All Threads
Download

newer

older

[Einstein Toolkit] #1540: Use...

[Einstein Toolkit] #1539:...

Einstein Toolkit

6 Feb 2014 6 Feb '14

1 p.m.

#1534: Bug in complex numbers ----------------------+----------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: new Priority: critical | Milestone: Component: Cactus | Version: development version Keywords: | ----------------------+----------------------------------------------------- I hear that C++ "complex<double>" and C "complex double" may have different ABI properties. This means that we need to correct Cactus's complex number implementation, probably using C's "complex double".

I hear that this makes Cactus complex numbers unusable on 32-bit Intel systems (untested).

See http://llvm.org/bugs/show_bug.cgi?id=18756 for details.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Show replies by date

Einstein Toolkit

6 Feb 6 Feb

2:07 p.m.

#1534: Bug in complex numbers -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: new Priority: critical | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+----------------------------------------------------

Comment (by knarf):

From what I read, I assume when you say

...

I hear that this makes Cactus complex numbers unusable on 32-bit Intel

systems (untested). you mean the current implementation, right? The problem seems to be that the C++ complex<double> is not (necessarily) ABI compatible to C's "complex double" - which is the same as Fortan's?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:1 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

2:10 p.m.

Comment (by knarf):

And just realizing this - it would also mean that the current complex number implementation is fine, if you don't use it from C++. Which of course should be corrected - just trying to understand the implications.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:2 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

2:16 p.m.

Comment (by eschnett):

I assume (but I don't know) that Fortran and C use the same ABI.

Yes, only C++ is broken, since Cactus implements the operations in C.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:3 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

5:56 p.m.

Comment (by rhaas):

Reading the bug report this seems to be only an issue for return values. The bug report states that even std::complex<double> is fine *in memory* ie std::complex<double> must look the same as _Complex double in memory. The only functions we have that returns a CCTK_COMPLEX are the CCTK_CmplxAdd etc functions. Since we now require C99 all of our supported languages (C, C++, F77, F90) support a complex type natively to do algebra with. So it seems feasible that we can make things standards conforming by removing the CCTK_CmplxAdd etc. functions which are no longer required since the users can just use the language provided functionality instead. For backwards compatibility we can provide macro versions of the routines in cctk_Math.h or similar (or inline functions returning CCTK_COMPLEX for C/C++). I actually do run the Cactus test suites on a 32bit (virtual) machine. The failing tests (current trunk) are (using gcc/gfortran): {{{ waveinterp-2p (from CarpetInterp) test_o9 (from CarpetProlongateTest) Schwarzschild_EF (from Exact) test_tov_carpet_refined_nosync (from GRHydro) tov_slowsector (from GRHydro) test_axibrill_nostagger (from IDAxiBrillBH) test_simpson (from Multipole) regression_test (from SphericalHarmonicRecon) }}} SphericalHarmonicRecon in PITTNull uses complex numbers. PITTNull itself is in Fortran so probably fine, MoL does the update and is in C so *should* be fine as well. There seems to be no C++ code using complex numbers in the tests. Slab uses complex numbers but the test does not use Slab. Both SphericalHarmonicReconGen and SphericalSlice do use them and are in C++, neither one is in the ET yet.

There are two reasons I can see for the SphericalHarmonicRecon test failing: 1. complex return values are handled differently in C and Fortran after all (different from what is stated in the link above) 2. 32bit machines by default compile for the i387 coprocessor which always uses 80bit long double precision in its registers (well almost always) while 64bit machines use sse2 which uses only 64bit double precision it its registers. One can test this by recompiling the code (assuming gcc) using -mfpmath=sse

I am testing patch for the flesh that replaces the complex algebra routines by inline function calls which should remove the problem of possibly differing return value representations and will report back on the results using the patch alone and using -mfpmath=sse.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:4 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

6:40 p.m.

Comment (by eschnett):

I checked: On a 32-bit system, C and Fortran use the same calling convention, and C++ differs. Thus it seems we can't use C++ complex numbers.

We currently typedef CCTK_COMPLEX differently in C and C++. I believe we need to change this, using "_Complex double" in both languages. Otherwise, e.g. a user-defined C routine returning CCTK_COMPLEX would fail when called from C++ and vice versa.

The best solution I can come up with is to introduce routines to the flesh that convert from/to CCTK_COMPLEX and complex<CCTK_REAL>. User code would then have to call these explicitly when accessing grid functions.

We can also introduce a new type CCTK_COMPLEX_CXX (namely complex<CCTK_REAL>), and use it to declare complex grid functions in C++. We can define this type even for C (as a struct, which has the correct memory layout). However, this would probably lead to trouble with header files included from both C and C++. I don't think this is a good idea.

As mentioned above, the standard operators should work in C++ even for C complex types (_Complex double). We could provide our own overloads of std::abs for these, so that C++ code "would not notice". Maybe these overloads should be in the cctk namespace instead.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:5 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

7:10 p.m.

#1534: Bug in complex numbers -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: review Priority: critical | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by rhaas):

* status: new => review

Comment:

On comment:5: introducing a struct type for CCTK_COMPLEX in C seem undesirable. We'd loose the ability to to arithmetic on them even when using C99. We'd also have no guarantee anymore that it will look the same as Fortran's complex numbers (though I'd be surprised if they would), nor does C++ seem to require that the compiler actually implements complex numbers as a user visible class. As far as I remeber C++ allows the compiler to recognize "#include <complex>" and then map std::complex<double> to some internal representation about which is does not have to tell us anything.

I checked the test results and the test failures in the ET when using 32bit machines are indeed due to the 80bit math precision. Compiling with -mfpmath=sse -march=prescott (and -O0 though I doubt that matters since it alone did not help) I get the test (in PITTNull) to pass.

Attached please find a patch for the flesh that removes the complex algebra helper routines and replaces them by inlined copies, one for C++ and one for C. This leaves the case where a user defines C routine that returns CCTK_COMPLEX but calls it from C++ where we I don't know which calling convention is used for a routine declared extern "C".

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:6 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

7 Feb 7 Feb

2:17 p.m.

Comment (by eschnett):

I attach an updated version of the patch that does not include <complex.h> from header files.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:7 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

3:28 p.m.

Comment (by rhaas):

Are the changes to Complex.c (compared to the original trunk version) required? It seems as if the new code does the same as the code it replaces only with fewer comments. The remove all the grdocs text which (in the flesh) are in our coding guide. If sufficient, I'd only change cctk_Complex.h to provide inline functions for C++ and the comment about not pushing "I" into global address space in front of the C versions.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:8 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

3:41 p.m.

Comment (by eschnett):

You are right; the changes to Complex.c should not be necessary.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:9 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

8 Feb 8 Feb

1:40 p.m.

Comment (by eschnett):

Do you agree with the complex.2.patch if Complex.c remains unchanged?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:10 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

3:13 p.m.

#1534: Bug in complex numbers -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reviewed_ok Priority: critical | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by rhaas):

* status: review => reviewed_ok

Comment:

Yes I agree with complex.2.patch if Complex.c remains unchanged. I attached such a version as complex.3.patch. Passes the tests on my 32bit virtual machine.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:11 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

11 Feb 11 Feb

3:27 p.m.

#1534: Bug in complex numbers -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: closed Priority: critical | Milestone: Component: Cactus | Version: development version Resolution: fixed | Keywords: -----------------------+---------------------------------------------------- Changes (by eschnett):

* status: reviewed_ok => closed * resolution: => fixed

Comment:

Applied.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1534#comment:12 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

4623

Age (days ago)

4628

Last active (days ago)

trac@lists.einsteintoolkit.org

12 comments

1 participants

tags (0)

participants (1)

Einstein Toolkit