-
Notifications
You must be signed in to change notification settings - Fork 11
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
Make LSODE integrator thread-safe in OpenMP parallel loops by adding THREADPRIVATE statements #121
base: dev
Are you sure you want to change the base?
Conversation
int/lsode.f90 - Now declare work arrays RWORK and IWORK as SAVEd and THREADPRIVATE in order to protect these variables from being overwritten by calls to LSODE on other threads in an OpenMP parallel loop. - Add THREADPRIVATE statements to declare common block /DLS001/ as private in an OpenMP parallel loop;. Likewise, this will prevent the common block variables from being overwritten by other threads. CHANGELOG.md - Updated accordingly Signed-off-by: Bob Yantosca <[email protected]>
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.
Thanks for asking me to review this. However, I don't have much
experience with OpenMP and "thread-safe" coding, so I will not be able
to contribute much here.
I spoke too soon. Compiling with GNU 12.2.0 resulted in this error: [ 68%] Building Fortran object src/GEOS-Chem/KPP/fullchem/CMakeFiles/KPP.dir/gckpp_Integrator.F90.o
/n/netscratch/jacob_lab/Lab/ryantosca/tests/kpptests/lsode/test_lsode/CodeDir/src/GEOS-Chem/KPP/fullchem/gckpp_Integrator.F90:254:27:
254 | INTEGER :: IWORK(LIW), IPAR(1), ITOL, ITASK, &
| ^
Error: ‘iwork’ causes a section type conflict with ‘rwork’
/n/netscratch/jacob_lab/Lab/ryantosca/tests/kpptests/lsode/test_lsode/CodeDir/src/GEOS-Chem/KPP/fullchem/gckpp_Integrator.F90:253:33:
253 | REAL(kind=dp) :: RWORK(LRW), RPAR(1)
| ^
note: ‘rwork’ was declared here I will look into this further. |
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.
Thanks @yantosca for these changes (and the helpful comments)! I am not sure about the section type conflict error or what could be causing it, so unfortunately I cannot help on that.
int/lsode.f90 - Added derived type declaration DLS001 - Added global variable CMN of type DLS001, which is threadprivate Work to be continued in subsequent commits Signed-off-by: Bob Yantosca <[email protected]>
int/lsode.f90 - Added more common variables to the CMN object, in order to hold these THREADPRIVATE for the OpenMP loop Signed-off-by: Bob Yantosca <[email protected]>
int/lsode.f90 - Removed unused SDIRK coefficients - Moved parameters LIW, LRW out of KppLsode to top-of-module, thus making these global parameters. - Added explanatory comments about the threadprivate CMN object replacing common block /DLS001/ - Bug fix: removed DLS from arg list to DLSODE routine - Restored variable declarations that were inadvertently removed during the removal of common block /DLS001/ - Trimmed trailing whitespace CHANGELOG.md - Updated accordingly Signed-off-by: Bob Yantosca <[email protected]>
int/lsode.f90 - The INIT, MXSTEP, MXHNIL, NHNIL, NSLAST, NYH variables were in common block /DLS001/ but were not added to the derived type DLS001 and object CMN. This is now fixed. - Added routine Zero_CMN_Fields to zero the fields of the CMN object before calling DLSODE - Updated comments - Trimmed trailing whitespace CHANGELOG.md - Updated accordingly Signed-off-by: Bob Yantosca <[email protected]>
@jimmielin: I wasn't able to make much headway with the section-type error. I decided to replace the |
Overview
This is the companion PR to #118. We have now made the LSODE integrator thread-safe by adding
$OMP THREADPRIVATE
statements to the following variables:RWORK
in routineKppLsode
(inint/lsode.f90
);SAVE
to make it a global variableIWORK
in routineKppLsode
(inint/lsode.f90
)SAVE
to make it a global variableAll instances of common block
/DLS001/
throughoutint/lsode.f90
Validation
I used our fork of the KPP-Standalone, and modified the
kpp_standalone.F90
driver program to include an OpenMP loop:This will perform an integration on the same initial conditions 10 times. The
C
array is overwritten by each integration. This is sufficient to test if there is a parallelization error.When compiled with OpenMP parallelization turned off, the integrations are done in order from 1..10 and we get this screen output:
final concentrations are written to the
ref.log
file. The difference inHexit
is because the initial conditions came from a run using the Rosenbrock integrator.Then I recompiled the executable with
-fopenmp
, and we get this screen output:Here you can see the instances are being done in non-sequential order, as the parallel loop is active. We get identical
Hexit
as the run with parallelization turned off. Also, the output indevmp.log
was identical toref.log
as determined by adiff
command.