-
Notifications
You must be signed in to change notification settings - Fork 539
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
BF OpenMp b4b #160
BF OpenMp b4b #160
Conversation
This reverts commit c471cda.
…jhenrique:/WW3/develop
…to fix OMP b4b irreproducibility issue (Jim Abeles)
…ly PRIVATE variables and avoid suprious assignment of values that lead to b4b reproducibility issues. Expanded matrix_ncep to run hybrid MPI/OMP cases
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.
@ajhenrique - It mostly looks good to me. I've left a few comments on specific lines.
…d T1 due to conflict in w3uqckmd OpenMP loop. Added note in manual switch.tex file.
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.
@ajhenrique There appears to be a lot of changes in the make_makefiles.sh file - more than just the change handling the mutual exclusion of T1 and OMPX switches. Was this intentional?
I.e. commit fb2b47b
… OMPH and T1 due to conflict in w3uqckmd OpenMP loop. Added note in manual switch.tex file." This reverts commit fb2b47b.
w3adc: Update to allow multiple switches on single line
Deleting NB and STKBND_INDEX
Both NB and STKBND_INDEX have been deleted. |
Removing parameters not used in specific OMP loops from PRIVATE declarations.
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.
I'm not expert in OMP so it would be better if Chris and Carsten can review this PR.
OK, agreed. Thanks @mickaelaccensi ! |
@CarstenHansen @ukmo-ccbunney Apologies for leaving this hanging. My regtests had not completed and hung for unknown reasons (checking with sysadmins). I had to restart and decided to rerun to ensure results are correct. I hope to complete that today and provide the results so we can finalize this PR review. In the meantime, what are the pending issues that need to be addressed? Thanks! |
@ajhenrique - if the multi-switch version of w3adc works and allows you to overcome the T1/OMPH issue, then I am happy to accept the review. |
Thanks @ukmo-ccbunney . Where should I expect to find an occurrence of a line with the double switch? |
@ajhenrique
Specifically the addition of the middle line. The modified w3adc program will then only insert the second line if both the T1 and OMPH switches are enabled. Edit: |
I still see two issues, which will probably not in practice appear very important:
|
Adding link to regtest matrix output from @ukmo-ccbunney for w3adc changes: |
@ukmo-ccbunney can you confirm this also requires a similar change in w3uno2md.ftn near line 818:
|
@CarstenHansen @ukmo-ccbunney I've completed the regression tests for this PR. I'd suggest that the request to change w3adc adding a limit to number of switches per line be made on a separate feature branch so we can move this PR in. Also, @mickaelaccensi would be making changes to w3iogo to address one of your comments. Please revise your comments and let me know of you approve the PR. In attachment you can find the matrixCompSummary file reflecting my regtests. |
@ajhenrique : yes, and also IY and IX need to be private (line 834/835), i.e:
|
|
This sounds perfect. |
@ajhenrique, I approve this PR
|
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.
Just one comment here, in response to your comment:
#160 (comment)
After that I think we are all ready to go.
@ukmo-ccbunney @CarstenHansen thanks for the productive review process. Iĺl be pushing this up within the next hour. Let me know if there are any issue left out. @aliabdolali this PR is going in today. |
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.
@ajhenrique Looks good to me!
Bugfix: Changes to several modules with OpenMP loops to declare properly PRIVATE variables and avoid spurious assignment of values that lead to b4b reproducibility issues. Expanded matrix_ncep to run hybrid MPI/OMP cases.