-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove CONTIGUOUS from dist_spec_control_argument #98
Conversation
These |
A bit more info: The job concerned is a 2PE job where the failure occurs on one of the PEs but not the other (the code then stops in the mpl_wait in dist_spec_control). The PSPECG argument is present on one PE and not present on the other. The PE that crashes is the one where the argument isn't present. The origin of this difference is in trans_distspec (in transi_module.F90) where there are two calls to dist_spec, one which passes PSPECG and the other with doesn't. |
Is this for a specific compiler only, or is this generally a problem? |
I am seeing failures in different downstream systems: saber (as noted above) and also lfric-lite-jedi. The saber failures are with gnu 9.2. For lfric-lite-jedi the failures occur with gnu 9.2 and gnu 12.2. It seems to work with gnu 7.3.0. Other systems would take some time to test as they are not fully ported. |
This is related to #95. |
Would a possibility be to add a compile time option to have the CONTIGUOUS attribute or not? It would make the code a bit ugly, but would allow people to have the attribute or not as works best for them. |
@marsdeno please could you liaise with author(s) of the CONTIGUOUS change? |
@DJDavies2 do you think this is related to #93? |
I guess doing something like
and then
might be acceptable? |
So a known working compiler is gfortran 7.3. |
@DJDavies2 do you have a simple reproducer I could try with latest GNU on our machine to see if the issue is still there? |
I don't have a simple reproducer I'm afraid. I can try to produce one if you like, but I can't guarantee I will be able to do so. |
I was thinking more like: #ifdef CONTIG_BUGGY_COMPILER but either way would be fine. The file already ends in .F90 so I think it is already being "pre-processed" even if their aren't any cpp statements at the moment. |
It is a different part of the code and a different compiler so I don't know.how connected these two issues are. It would certainly be helpful if #93 could be handled. |
The advantage of defining something like CONTIG_STATUS is it could be done once, and used wherever needed. In fact it could be done in one of the CMake files. |
So what's missing now is a CMake modification to add the preprocessor definition |
I wasn't quite sure how to approach this. I don't know exactly which compilers/versions would need this option to be set and it could get messy and error-prone to try and handle it in the cmake file. An alternative would be to just let users set the option themselves via CMAKE_Fortran_FLAGS if they need it. What do you think? |
Something roughly like
in the main CMakeLists.txt and adding
after the ecbuild_add_library() call in src/trans/CMakeLists.txt ? |
Good @marsdeno except I'd perhaps use the variable |
@DJDavies2 I applied my suggestion on top of your latest change in |
Thanks, I have tested the latest version of this branch and it seems to work for us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! This looks like ready to merge then.
* develop: Add GPU capability (ecmwf-ifs#106 , ecmwf-ifs#120)
I am not entirely sure what is happening here, but this change does seem to fix the issue at least for the failing job in question. The change just removes the contiguous attribute from one of the arguments (PSPECG). The other argument with the contiguous attribute, PSPEC, seems to be fine, but I don't know why there would be a difference. The only obvious difference is the intent.