Conversation
Changed the default integer type for grid sizes from int to a 64bit signed integer type on 64bit systems. This enables using large grids with more than 2^31 grid points. For maintainability a new typedef meep::integer is introduced in meep/meeptyes.hpp and is set to ptrdiff_t. This will ensure the proper sizes on both 32bit and 64bit systems. The changes were made according to the following procedure: - the type of meep::grid_volume::the_ntot was updated to meep::integer - evrything was recompiled with -Wconversion under gcc4.8.2 on a 64bit system - iteratively all places with an implicit conversions from meep::integer to a smaller interger type where updated until all warnings disappeared - loop variables were updated from int to meep::integer when a comparison against meep::integer was involved in the iteration - all casts to int were checked and corrected where necessary - additional casts were introduced to silence conversion warnings fron or to double - print format specifiers were chenge from %d to %ld with additional casts to long int to avoid warnings on 32bit systems - MPI communication wrappers were updated to accept meep::integer as message size. As MPI specifies only int size messages (and the messages hopefully will not exceed this limit), internally the size is just downcasted to int. However, checks of the message size were added before downcasting to abort when the message size does not allow a safe cast. This at least ensures a proper error message when the supported message size is exceeded.
There was a problem hiding this comment.
Those casts were inserted to silence compiler warnings. Converting from a meep::interger to a double is a narrowing operation on a 64 bit system (the double mantissa has only 52 bits). To find all potential problems with the new 64 bit int type I had to use the gcc option -Wconversion that warns when an implicit conversion may alter the value. The meep::integer -> double conversions gave hundreds of false positives which made it hard to find the real problems. So I decided to insert static_cast<double> to make the conversion explicit. It turns off the compiler warning and documents that the conversion is actually intented. It looks a bit ugly but it does not cause any overhead (the conversion has to happen anyway, no matter if it is implicit or explicit). So I would prefer to leave the casts.
|
A lot of the type conversions here seem superfluous, e.g. the calls to |
|
The calls to Some of the promotions from |
|
Fair enough. |
src/mympi.cpp
Outdated
There was a problem hiding this comment.
inconsistent indentation here.
There was a problem hiding this comment.
Since you have this check several times, it would be better to write a function:
static void check_size(meep::integer size) {
if (size > INT32_MAX) abort("MPI message size exceeds maximum");
}and then just call check_size.
There was a problem hiding this comment.
Sounds good. I include the check into the cast operation by replacing static_cast<int>(size) with a separate function cast_message_size(size).
src/mympi.cpp
Outdated
There was a problem hiding this comment.
Seems like there should be a check somewhere that sizeof(long int) == sizeof(meep::integer).
There was a problem hiding this comment.
As long as sizeof(long int) >= sizeof(meep::integer) everything is fine because the data of pt is copied. However, on systems with LLP64 data model (are there any except MS Windows?) there will be trouble (maybe also in other places?).
One way to resolve the issue would be to use long long int instead of long int.
Another way would be to use meep::integer and define a suiteable MPI data type, e.g. MPI_MEEP_INTEGER, that points either to MPI_INT, MPI_LONG or MPI_LONG_LONG_INT, depending on the size of meep::integer. Then it would be possible to write
ivec max_to_all(const ivec &pt) {
meep::integer in[5], out[5];
for (int i=0; i<5; ++i) in[i] = out[i] = pt.in_direction(direction(i));
#ifdef HAVE_MPI
MPI_Allreduce(&in,&out,5,MPI_MEEP_INTEGER,MPI_MAX,mycomm);
#endif
ivec ptout(pt.dim);
for (int i=0; i<5; ++i) ptout.set_direction(direction(i), out[i]);
return ptout;
}I will try this and see how far I can get with AC_CHECK_SIZEOF and the preprocessor.
- added a define MPI_MEEP_INTEGER that maps to the appropriate MPI type - added mpi communication wrappers and auxiliary functions for long long int (if the type is supported by the compiler)
|
I added precautions for the case |
|
Looking good. I assume you've tried |
|
|
|
Could it be that your LAPACK library is providing its own BLAS? (Sometimes |
This reverts commit a034638.
|
No, my LAPACK lib is not providing its own BLAS. I checked this. I suspect it is some issue with |
|
Yes, I ran |
|
Reducing the optimization level from |
|
Was there a measurable performance penalty to turning off auto-vectorization? (Probably this flag only helps in |
|
I tested mainly with gcc 4.8 and 4.9 and to some minor extent with gcc 5. All versions show the same behavior. The test failures occur when sse2 or avx are activitated either by using |
|
#193 is a more minimal version of this change that hopefully addresses the needs for large grids. |
2 participants
This is a quite invasive commit to enable very large grids with more than 2^31-1 grid points. Basically the changes consist of setting the type of
meep::grid_volume::the_ntotand all related variables toptrdiff_t(via a typedef) and tracking down all errors, conversion warnings and loop variables (for details see the commit message of 521cb13).So far the changes have been tested only on x86-64 with gcc 4.8 and 4.9. I ran some simulations with very large grids (>2^31-1 grid points) for a dispersive gold grating (x and y periodic, z pmls) and a dipole emitter in a large box terminated with pmls. The obtained resuts were consistent with smaller grid sizes and unchanged versions of meep. I can provide both the results and the ctl files if you are interested. If there are any further tests you would like me to perform please let me know.