- removed comment
Compile with adaptive MPI (based on Charm++)
Issue #1618
new
Gengbin Zheng, one of the leading developer of the Charm++ and I managed to get Cactus compile against the AMPI (http://charm.cs.illinois.edu/research/ampi). The patch against Revsion 5120 is attached. The major changes are in the flesh to deal with global variables. We started with PUGH to get started and changes in Carpet could be done later on. NR applications might not benefit from this work though this effort could make Cactus appealing to many other applications as a computational framework.
Keyword:
Comments (12)
-
-
- removed comment
We are always happy to increase the user base for Cactus. Supporting AMPI seems like a nice thing to have. The patch as it is can likely not be applied to the public version of Cactus though. We most likely have to replace the AMPI_TLS defines with a CCTK_PROCLOCAL or similar and define it to be empty if AMPI is not used (the trickiest issue being that we'd want this all to work if either OpenMP or AMPI are used so we cannot just make AMPI_TLS into
__thread). Deciding whether AMPI is in use could be done either in the flesh (ideally to be avoided since the flesh should not know of MPI) or the thorn ExternalLibraries/MPI. It would also be very nice to limit calls to CmiEnableTLS/CmiDisableTLS to only the two places namely where we call MPI_Init and MPI_Finilize. A second issue is that the patch currently contains a line (line 562 of WarnLevel.c that says "/ Gengbin hack /" which we have to remove). Am I correct in assuming that this particular line is related to #1548 ? -
- removed comment
Yes, OpenMP is using __thread internally. That is why Gengbin added another macro instead of using __thread directly. Yes, we may find a more systematic approach to handle thread local storage, which might also be necessary for compiling Cactus with HPX (http://stellar.cct.lsu.edu/tag/hpx/). Yes, #1548 is relevant to this patch which has a dirty hack to get around the issue in that ticket.
-
- removed comment
Do we like this patch? Do we need to split it up into smaller pieces?
-
- removed comment
No, we do not like this patch:
AMPI_TLS needs to be named CCTK_TLS. Possibly, "TLS" should be "THREAD_LOCAL" instead, so that it is more similar to the modern C and C++ standards.
The construct
#ifdef AMPI CmiDisableTLS(); #endifshould be wrapped into a single function, probably called "CCTK_DisableThreadLocal()" or so. Same for CmiEnableTLS. In particular, the #ifdefs should go into this function, so that using it requires only 1 instead of 3 lines of code.
The various comments containing the work "hack" should instead contain an explanation of what they do. There is no need to call it "hack".
"AMPI" should become "CCTK_HAVE_AMPI", similar to other capabilities.
The header file mpi.h should be accessed via <mpi.h>, not "mpi.h".
There are some commented out lines of code that are introduced. These should not be introduced.
The code in Boundary.c seems fishy. Are these static variables really correct, even if executed by multiple threads? Does the code need to be redesigned instead?
shelob.ini should not change the default for Shelob.
The newly introduced CCTK_* macros and functions need to be documented.
But before we do any of this, it would be good to describe in an email to the developers' list which new features you propose to introduce to the flesh, and why, and what other packages (except AMPI) may want similar features, how this compares to C11 and C++11, etc.
-
- removed comment
The code in Boundary.c seems fishy. Are these static variables really correct, even if ?>executed by multiple threads? Does the code need to be redesigned instead?
Erik, would you please explain a bit more about your concerns? Other changes shall be straightforward.
Jian
-
- removed comment
I just attached the updated patch from Gengbin. Jian
-
- removed comment
Replaced AMPI_TLS with CCTK_TLS.
-
- removed comment
Steve: more changes have to be made, but it is on the right track.
-
- removed comment
Brief notes from the phone discussion:
- rename TLS to THREAD_LOCAL (both macro and include file)
- define CCTK_THREAD_LOCAL via autoconf magic, choose whether to use this macro depending on whether AMPI is used
- add documentation for Enable/DisableThreadLocal
- simplify logic for redirecting I/O: don't introduce another else branch, rather move #ifdef into else branch
- declare CmiEnableTLS only when AMPI is available
- why is mpi.h needed for flesh.cc?
- don't use -DCCTK_TLS in option list
- don't use -DCCTK_HAVE_AMPI in option list; rather set this in AMPI thorn
-
- removed comment
With regard to the point "add documenation for Enable/Disable" it was requested that documentation for why thread local storage needs to be disabled where it is disabled.
-
- removed comment
I meant that the reference manual needs to be updates with a description of these routines. The comments in the code can then assume that the reader is familiar with this documentation.
- Log in to comment
- Assignee
- –
- Type
- Priority
- Status
- new
- Component
- Cactus
- Milestone
- –
- Version
- development version
- Votes
- 0
- Watchers
- 0
The reason Jian says that NR may not benefit is that simulation with large number of ghost zones may not benefit. Switching to other numerical methods, such as DG, may change the picture.
Re Carpet: Most of Carpet's global variables live in a namespace. The reason this was done was one of convenience -- it is easier to access a namespace than a singleton class object where the pointer has to be retrieved from cctkGH. The changes to avoid global variables are straightforward, but tedious.
There exist other global variables having to do with profiling, timing, keeping statistics etc. Those should probably be put into thread-local storage.