Cactus: New cGH fields tile_min, tile_max
CarpetX’s OpenMP parallelization scheme tiles components, and calls scheduled functions once per tile. The new cGH fields tile_min
and tile_max
describe the local tile.
See https://bitbucket.org/cactuscode/cactus/pull-requests/104/cactus-new-cgh-fields-tile_min-tile_max
Comments (11)
-
-
-
assigned issue to
-
assigned issue to
-
reporter I corrected the code.
-
I am afraid this may require more work. Sorry for the piecemeal review (though some of the comments were present before):
- this changes the
omp single
clause by removing thecopyprivate(cctki3_bndsize, cctki3_is_physbnd)
which will leavecctki3_bndsize
uninitialized in the threads that do not execute the single clause it seems since those arrays are automatic, private variables. They could be made static though it seems, which hopefully won’t make themthreadprivate
. - the prototype of
GetBoundarySizesAndTypes
should not be duplicated. Thorns usingCCTK_LOOPX_INT
must themselvesUSE
orREQUIRE
the aliased function at which point a prototype will be available when the macro is expanded - the
#else #define CCTK_LOOP_TILE_MIN(cctkGH, d) 0 #define CCTK_LOOP_TILE_MAX(cctkGH, d) (cctkGH)->cctk_lsh[d]
- does not seem to be required since both
cctk_Loop.h
andcctk_GroupsOnGH.h
are in the same repository so will be kept in sync by git - no support for Fortran is provided (F90 is still in use, see http://lists.einsteintoolkit.org/pipermail/users/2020-February/007298.html)
- this changes the
-
Looking at the
omp single
and in particular its impliedomp barrier
at the end, I suspect the current code may also run the risk of creating a deadlock when the scheduled functions are called in a multi-threaded way inCarpetX
. Namely since thebarier
assumes that all threads will encounter it, there would be a deadlock if the the number of tiles processed by each thread is not identical to that of any of the other threads. A possible workaround seems to be to remove the barrier by usingnowait
which however makescopyprivate
impossible requiring that thecctki_bndsize
arrays bestatic
so that they are shared among the threads. -
reporter Code is even moar correct now.
-
Removed prototype for
GetBoundarySizesAndTypes
-
Removed parallelization for
GetBoundarySizesAndTypes
- Removed CCTK_LOOP_TILE_* macros
- Added Fortran support
-
-
I like the new pull request with more correctness than ever.
Please apply.
-
-
- changed status to resolved
-
Having approved and merged I now (naturally) notice this in cGH:
/* unused */ int *cctk_to; int *cctk_from;
which could have been used instead of
tile_min
andtile_max
. -
reporter I knew about these, and I looked at using them instead of defining new fields. I decided it was not possible. Unfortunately I don’t recall the reason why.
- Log in to comment
Currently this seems not ready to be applied.