Skip to content
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

Adding OpenACC directives to top of split time integration routine #4939

Merged

Conversation

brobey
Copy link
Contributor

@brobey brobey commented May 6, 2022

Adding OpenACC to top of split time integration routine. This is mostly adding directives and should not impact anything else. Tested on Darwin with NVHPC (OpenACC), Intel, and GCC with bfb compared to head. Could not test on Cori-KNL - no longer supported in compass. Tried Cori-Haswell, but getting three fails with master and with changes. Still don't have a Chrysalis or Summit account to test on those platforms. Broke prior OpenACC extend split pull request in the process so will have to recreate and repost those changes.

[BFB]

@jonbob jonbob self-assigned this May 6, 2022
@jonbob
Copy link
Contributor

jonbob commented May 6, 2022

@brobey - can you please change the title of this PR to something more descriptive?

@brobey brobey changed the title Brobey/mpas ocean/openacc topofsplit Adding OpenACC directives to top of split time integration routine May 6, 2022
Copy link
Contributor

@philipwjones philipwjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the specific bug fix needed for Summit.

Other (optional) changes. For OpenACC on Summit, we don't see any benefit from a parallel region (that is, separating parallel from the associated loop construct) so I guess I have a preference for keeping "parallel loop" as a combined directive and local present variables rather than having the separate parallel/end parallel and individual loop constructs. Were you seeing an advantage to splitting these on other machines?

Also, an optional cosmetic thing - would prefer to keep line lengths in the 75-80 char range to fit a terminal window so would prefer to break up long lines with continuation chars, even for directives.

We can reduce clutter a little by only using ifdefs for OpenACC when we need to distinguish from OpenMP for loop directives. Again, just a personal preference.

Copy link
Contributor

@philipwjones philipwjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved after verifying most recent changes are bfb on Summit and continue to pass PR tests on others. And thanks for the additional cleanup.

Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I compiled with gnu optimized, and it matches bfb with the previous commit for MPAS standalone nightly suite. Also tested with gnu debug.

@jonbob jonbob added the BFB PR leaves answers BFB label May 26, 2022
jonbob added a commit that referenced this pull request May 26, 2022
Adding OpenACC directives to top of split time integration routine

Adding OpenACC to top of split time integration routine. This is mostly
adding directives and should not impact anything else.

[BFB]
@jonbob
Copy link
Contributor

jonbob commented May 26, 2022

passes sanity tests, merged to next

@jonbob jonbob merged commit 9281eda into E3SM-Project:master May 27, 2022
@jonbob
Copy link
Contributor

jonbob commented May 27, 2022

merged to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB mpas-ocean Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants